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
7 changes: 7 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,13 @@
const results = await registry.import(source, { basePath, validate: true });
for (const result of results) {
if (!result.valid && result.errors) {
// Check if error is just "already exists" - skip silently

Check notice on line 553 in src/config.ts

View check run for this annotation

probelabs / Visor: security

logic Issue

The 'already exists' error handling logic is duplicated across multiple files (config.ts, workflow-check-provider.ts). This creates maintenance burden and potential for inconsistent behavior.
Raw output
Extract the duplicate 'already exists' error handling into a shared utility function to ensure consistent behavior across all workflow import locations.

Check warning on line 553 in src/config.ts

View check run for this annotation

probelabs / Visor: quality

error-handling Issue

Silent skipping of 'already exists' errors during workflow import could hide real problems (e.g., partial imports, validation failures masked by duplicate detection).
Raw output
Add a counter or metric to track skipped imports, and log a warning if too many are skipped: logger.warn(`Skipped ${skippedCount} duplicate workflow imports`);
// This allows multiple workflows to import the same dependency
const isAlreadyExists = result.errors.every(e => e.message.includes('already exists'));
if (isAlreadyExists) {
logger.debug(`Workflow from '${source}' already imported, skipping`);
continue;
}
const errors = result.errors.map(e => ` ${e.path}: ${e.message}`).join('\n');
throw new Error(`Failed to import workflow from '${source}':\n${errors}`);
}
Expand Down
53 changes: 47 additions & 6 deletions src/frontends/slack-frontend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@
const ev = (env && env.payload) || env;
// For chat-style AI checks, post direct replies into the Slack thread
await this.maybePostDirectReply(ctx, ev.checkId, ev.result).catch(() => {});
// Post execution failure notices when a check completes with fatal issues
await this.maybePostExecutionFailure(ctx, ev.checkId, ev.result).catch(() => {});
})
);
this.subs.push(
Expand Down Expand Up @@ -260,7 +262,6 @@
checkId?: string
): Promise<void> {
if (this.errorNotified) return;
if (!this.isTelemetryEnabled(ctx)) return;
const slack = this.getSlack(ctx);
if (!slack) return;
const payload = this.getInboundSlackPayload(ctx);
Expand All @@ -273,9 +274,11 @@
if (checkId) text += `\nCheck: ${checkId}`;
if (message) text += `\n${message}`;

const traceInfo = this.getTraceInfo();
if (traceInfo?.traceId) {
text += `\n\n\`trace_id: ${traceInfo.traceId}\``;
if (this.isTelemetryEnabled(ctx)) {
const traceInfo = this.getTraceInfo();
if (traceInfo?.traceId) {
text += `\n\n\`trace_id: ${traceInfo.traceId}\``;
}
}

const formattedText = formatSlackText(text);
Expand All @@ -288,17 +291,55 @@
this.errorNotified = true;
}

private isExecutionFailureIssue(issue: any): boolean {

Check warning on line 294 in src/frontends/slack-frontend.ts

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

Timeout detection logic is scattered across multiple files (slack-frontend.ts, routing.ts, level-dispatch.ts, stats-manager.ts, command-check-provider.ts). Each location implements slightly different patterns for detecting timeouts via ruleId.includes('timeout') or message text matching, creating inconsistency and maintenance burden.
Raw output
Centralize timeout detection logic into a single utility function (e.g., isTimeoutIssue(issue: any): boolean) that can be imported and reused across all components. This ensures consistent timeout classification and reduces code duplication.

Check warning on line 294 in src/frontends/slack-frontend.ts

View check run for this annotation

probelabs / Visor: quality

architecture Issue

The isExecutionFailureIssue method uses multiple string matching patterns that are duplicated across the codebase (routing.ts has similar logic). This creates maintenance burden.
Raw output
Extract to a shared utility: src/utils/issue-classifiers.ts with functions like isExecutionFailureIssue(), isTimeoutIssue(), isLogicalFailureIssue().
const ruleId = String(issue?.ruleId || '');
const msg = String(issue?.message || '');
const msgLower = msg.toLowerCase();
return (
ruleId.endsWith('/error') ||
ruleId.includes('/execution_error') ||
ruleId.includes('timeout') ||
ruleId.includes('sandbox_runner_error') ||
msgLower.includes('timed out') ||
msg.includes('Command execution failed')
);
}

private async maybePostExecutionFailure(
ctx: FrontendContext,
checkId: string,
result: { issues?: any[] }
): Promise<void> {
try {
if (this.errorNotified) return;
const cfg: any = ctx.config || {};
const checkCfg: any = cfg.checks?.[checkId];
if (!checkCfg) return;
if (checkCfg.criticality === 'internal') return;
const issues = (result as any)?.issues;
if (!Array.isArray(issues) || issues.length === 0) return;

const failureIssue = issues.find(issue => this.isExecutionFailureIssue(issue));
if (!failureIssue) return;

Check warning on line 324 in src/frontends/slack-frontend.ts

View check run for this annotation

probelabs / Visor: security

security Issue

Empty catch block at line 324 in maybePostExecutionFailure() silently suppresses all errors. This could prevent users from being notified about execution failures if the error posting logic itself fails.
Raw output
Add logging to ensure errors are not silently swallowed: } catch (error) { logger.error('[SlackFrontend] Failed to post execution failure:', error); }
const msg =
typeof failureIssue.message === 'string' && failureIssue.message.trim().length > 0
? failureIssue.message.trim()

Check failure on line 327 in src/frontends/slack-frontend.ts

View check run for this annotation

probelabs / Visor: quality

error-handling Issue

Empty catch block in maybePostExecutionFailure silently suppresses all errors. This could prevent critical error notifications from being posted.
Raw output
Add error logging: catch (error) { logger.error('[SlackFrontend] Failed to post execution failure:', error); }
: `Execution failed (${String(failureIssue.ruleId || 'unknown')})`;
await this.maybePostError(ctx, 'Check failed', msg, checkId);
} catch {}
}

private async ensureAcknowledgement(ctx: FrontendContext): Promise<void> {
if (this.acked) return;
const ref = this.getInboundSlackEvent(ctx);
if (!ref) return;
const slack = this.getSlack(ctx);
if (!slack) return;
// Skip ack for bot messages to avoid loops
// Skip ack for our own bot messages to avoid loops (allow other bots)
try {
const payload = this.getInboundSlackPayload(ctx);
const ev: any = payload?.event;
if (ev?.subtype === 'bot_message') return;
// If we can resolve bot user id, skip if the sender is the bot
try {
const botId = await slack.getBotUserId?.();
Expand Down
9 changes: 4 additions & 5 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -516,12 +516,13 @@ async function handleEvent(
}

// Check if this is a bot comment that we should skip
// Only skip Visor's own bot accounts and comments with Visor markers
// Allow other bots to trigger Visor for integration purposes
const comment: CommentLike | undefined = context.event?.comment;
const shouldSkipBotComment =
comment &&
(comment.user?.login === 'visor[bot]' ||
comment.user?.login === 'github-actions[bot]' ||
comment.user?.type === 'Bot' ||
(comment.body && comment.body.includes('<!-- visor-comment-id:')));

// Extract context for reactions
Expand Down Expand Up @@ -984,11 +985,9 @@ async function handleIssueComment(
}

// Prevent recursion: skip if comment is from visor itself
// Check both comment author && content markers
// Only skip Visor's own bot accounts, allow other bots to trigger Visor
const isVisorBot =
comment.user?.login === 'visor[bot]' ||
comment.user?.login === 'github-actions[bot]' ||
comment.user?.type === 'Bot';
comment.user?.login === 'visor[bot]' || comment.user?.login === 'github-actions[bot]';

const hasVisorMarkers =
comment.body &&
Expand Down
6 changes: 6 additions & 0 deletions src/providers/command-check-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,12 @@
isTimeout = true;
}
}
// Fallback: detect timeout based on error message text

Check failure on line 1148 in src/providers/command-check-provider.ts

View check run for this annotation

probelabs / Visor: quality

architecture Issue

The execute method is over 400 lines long and handles too many responsibilities (template rendering, command execution, error handling, output parsing, transformation). This violates Single Responsibility Principle.
Raw output
Extract logical sections into separate methods: renderCommandTemplate(), parseCommandOutput(), applyTransforms(), extractIssues(), handleExecutionError().
if (!isTimeout && error instanceof Error) {
if (/timed out/i.test(error.message)) {
isTimeout = true;
}
}

// Extract stderr from the error if available (child_process errors include stdout/stderr)
let stderrOutput = '';
Expand Down
37 changes: 19 additions & 18 deletions src/providers/workflow-check-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import { generateHumanId } from '../utils/human-id';
// eslint-disable-next-line no-restricted-imports -- needed for Liquid type
import { Liquid } from 'liquidjs';
import { createExtendedLiquid } from '../liquid-extensions';

/**
* Provider that executes workflows as checks
Expand All @@ -26,7 +27,7 @@
super();
this.registry = WorkflowRegistry.getInstance();
this.executor = new WorkflowExecutor();
this.liquid = new Liquid();
this.liquid = createExtendedLiquid();

Check notice on line 30 in src/providers/workflow-check-provider.ts

View check run for this annotation

probelabs / Visor: performance

performance Issue

Creating new Liquid instance with createExtendedLiquid() in constructor is efficient, but ensure the instance is reused across multiple executions rather than recreated
Raw output
The current implementation correctly reuses the instance. This is informational - the pattern is correct.

Check notice on line 30 in src/providers/workflow-check-provider.ts

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The change from 'new Liquid()' to 'createExtendedLiquid()' is applied inconsistently across the codebase. While workflow-check-provider.ts, workflow-executor.ts, and test-runner/index.ts now use createExtendedLiquid(), other parts of the system may still be using plain Liquid instances, leading to inconsistent feature availability.
Raw output
Conduct a comprehensive audit of all Liquid instantiation across the codebase to ensure createExtendedLiquid() is used consistently wherever Liquid templates are rendered. Document the standard approach in a central location to prevent future inconsistencies.
}

getName(): string {
Expand Down Expand Up @@ -792,26 +793,26 @@
if (rawData.imports && Array.isArray(rawData.imports)) {
const configDir = path.dirname(resolved);
for (const source of rawData.imports) {
try {
const results = await this.registry.import(source, {
basePath: configDir,
validate: true,
});
for (const result of results) {
if (!result.valid && result.errors) {
const errors = result.errors.map((e: any) => ` ${e.path}: ${e.message}`).join('\n');
throw new Error(`Failed to import workflow from '${source}':\n${errors}`);
const results = await this.registry.import(source, {

Check failure on line 796 in src/providers/workflow-check-provider.ts

View check run for this annotation

probelabs / Visor: quality

architecture Issue

The 'already exists' error handling logic is duplicated from config.ts and workflow-registry.ts. This is the third instance of the same pattern.
Raw output
URGENT: Extract to shared utility function to prevent further duplication. The pattern has been copied 3 times in this PR alone.
basePath: configDir,
validate: true,
});
for (const result of results) {
if (!result.valid && result.errors) {
// Check if error is just "already exists" - skip silently
// This allows multiple workflows to import the same dependency
const isAlreadyExists = result.errors.every((e: any) =>
e.message.includes('already exists')
);
if (isAlreadyExists) {
logger.debug(`Workflow from '${source}' already imported, skipping`);
continue;
}
}
logger.info(`Imported workflows from: ${source}`);
} catch (err: any) {
// If the workflow already exists, log a warning but don't fail
if (err.message?.includes('already exists')) {
logger.debug(`Workflow from '${source}' already imported, skipping`);
} else {
throw err;
const errors = result.errors.map((e: any) => ` ${e.path}: ${e.message}`).join('\n');
throw new Error(`Failed to import workflow from '${source}':\n${errors}`);
}
}
logger.info(`Imported workflows from: ${source}`);
}
}

Expand Down
84 changes: 70 additions & 14 deletions src/slack/socket-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,22 +165,46 @@

private async handleMessage(raw: string): Promise<void> {
let env: any;
try {

Check notice on line 168 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The Slack socket filtering logic now contains extensive debug logging blocks (VISOR_DEBUG checks) that are repeated for every filter condition. While useful for debugging, this pattern creates code bloat and reduces readability by mixing logging concerns with business logic.
Raw output
Consider extracting the debug logging into a single conditional wrapper function or using a debug logging utility that conditionally logs based on the VISOR_DEBUG flag. This would reduce code duplication and improve readability of the core filtering logic.
env = JSON.parse(raw);
} catch {
if (process.env.VISOR_DEBUG === 'true') {

Check notice on line 171 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: performance

performance Issue

Empty catch block after ensureClient() could hide errors and cause performance issues if client initialization fails repeatedly
Raw output
Add logging to the catch block to track client initialization failures. This helps diagnose performance issues from repeated failed attempts.
logger.debug('[SlackSocket] Dropping non-JSON payload');
}
return;
}
if (env.type === 'hello') return;
if (env.envelope_id) this.send({ envelope_id: env.envelope_id }); // ack ASAP
if (env.type !== 'events_api' || !env.payload) return;
if (env.type !== 'events_api' || !env.payload) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(`[SlackSocket] Dropping non-events payload: type=${String(env.type || '-')}`);
}
return;
}
const p = env.payload;
const ev = p.event || {};
const type = ev.type as string;
const subtype = ev.subtype as string | undefined;

// Ensure botUserId is available for mention detection when possible
try {
await this.ensureClient();

Check warning on line 191 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: security

security Issue

The bot message filtering logic was changed to allow third-party bots to trigger Visor. While this enables integrations, the new logic only checks if the message is from Visor's own bot. This could allow malicious bots to spam Visor or trigger unwanted actions, especially in channels where mentions are not required.
Raw output
Implement an allowlist of trusted bot IDs that can trigger Visor, or add a configuration option to control which bots are allowed to interact with the system.

Check warning on line 191 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: security

security Issue

Empty catch block at line 191 silently suppresses errors from ensureClient(). This could hide authentication failures or configuration issues that prevent proper botUserId detection, weakening the security controls that depend on it.
Raw output
Add logging to the catch block: } catch (error) { logger.warn('[SlackSocket] Failed to ensure client for botUserId detection:', error); }
} catch {}

// Ignore bot messages and edited/changed events to prevent loops
if (ev.subtype && ev.subtype !== 'thread_broadcast') return;
if (ev.bot_id) return;
if (this.botUserId && ev.user && String(ev.user) === String(this.botUserId)) return;
// Ignore edited/changed events to prevent loops
if (subtype && subtype !== 'thread_broadcast' && subtype !== 'bot_message') {

Check warning on line 195 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: quality

error-handling Issue

Empty catch block after ensureClient() call silently suppresses errors. This could hide initialization problems and make debugging difficult.
Raw output
Add logging to the catch block: catch (error) { logger.debug('[SlackSocket] Failed to ensure client:', error); }
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(`[SlackSocket] Dropping subtype=${subtype}`);
}
return;
}
// Only skip our own bot messages, allow other bots to trigger Visor
if (this.botUserId && ev.user && String(ev.user) === String(this.botUserId)) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug('[SlackSocket] Dropping self-bot message');
}
return;
}

// Info-level receipt log for observability
try {
Expand All @@ -196,30 +220,62 @@
} catch {}

// Gating: mentions / DM vs channel
const channelId = String(ev.channel || '');

Check failure on line 223 in src/slack/socket-runner.ts

View check run for this annotation

probelabs / Visor: quality

error-handling Issue

Multiple empty catch blocks in message handling logic (lines 223, 233, 243, 252, 261, 270) suppress errors without logging. This pattern makes it impossible to diagnose issues in production.
Raw output
Add debug logging to all catch blocks: catch (error) { if (process.env.VISOR_DEBUG === 'true') { logger.debug('[SlackSocket] Error in X:', error); } }
const isDmLike = channelId.startsWith('D') || channelId.startsWith('G');
// Only treat 1:1 DMs (D*) as DM-like; private channels/MPIMs (G*) require mentions.
const isDmLike = channelId.startsWith('D');
const text = String(ev.text || '');
const hasBotMention = !!this.botUserId && text.includes(`<@${String(this.botUserId)}>`);

// In public channels (C*), only react to app_mention events so we don't
// trigger on every message in a thread. In DMs/MPIMs we allow plain
// message events when mentions=all.
// In public (C*) and private (G*) channels, require an explicit bot mention
// so we don't trigger on every message in a thread.
// In 1:1 DMs we allow plain message events when mentions=all.
if (!isDmLike) {
if (type !== 'app_mention') return;
if (!hasBotMention) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(
`[SlackSocket] Dropping channel message: type=${type} hasBotMention=${hasBotMention}`
);
}
return;
}
} else {
if (this.mentions === 'direct' && type !== 'app_mention') return;
if (this.mentions === 'direct' && type !== 'app_mention' && !hasBotMention) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(
`[SlackSocket] Dropping DM message: type=${type} hasBotMention=${hasBotMention}`
);
}
return;
}
}
// Gating: threads (accept root messages by treating ts as thread root)
const rootOrThread = ev.thread_ts || ev.ts || ev.event_ts;
if (this.threads === 'required' && !rootOrThread) return;
if (this.threads === 'required' && !rootOrThread) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug('[SlackSocket] Dropping message: threads required but no thread_ts');
}
return;
}
// channel allowlist
if (!this.matchesAllowlist(ev.channel)) return;
if (!this.matchesAllowlist(ev.channel)) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(`[SlackSocket] Dropping message: channel not in allowlist`);
}
return;
}

// Deduplicate duplicate events (e.g., message + app_mention with same ts)
try {
const key = `${String(ev.channel || '-')}:${String(ev.ts || ev.event_ts || '-')}`;
const now = Date.now();
for (const [k, t] of this.processedKeys.entries())
if (now - t > 10000) this.processedKeys.delete(k);
if (this.processedKeys.has(key)) return;
if (this.processedKeys.has(key)) {
if (process.env.VISOR_DEBUG === 'true') {
logger.debug(`[SlackSocket] Dropping duplicate event key=${key}`);
}
return;
}
this.processedKeys.set(key, now);
} catch {}

Expand Down
1 change: 1 addition & 0 deletions src/state-machine/dispatch/stats-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
return (
ruleId.endsWith('/error') ||
ruleId.includes('/execution_error') ||
ruleId.includes('timeout') ||

Check failure on line 12 in src/state-machine/dispatch/stats-manager.ts

View check run for this annotation

probelabs / Visor: quality

architecture Issue

The hasFatalIssues function is duplicated in stats-manager.ts and level-dispatch.ts with slightly different logic. This creates maintenance burden and risk of inconsistent behavior.
Raw output
Consolidate into a single shared utility function in a common module: src/state-machine/utils/fatal-issues.ts
ruleId.endsWith('_fail_if')
);
});
Expand Down
5 changes: 5 additions & 0 deletions src/state-machine/states/level-dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,7 @@
return (
ruleId.endsWith('/error') || // System errors
ruleId.includes('/execution_error') || // Command failures
ruleId.includes('timeout') || // Timeouts
ruleId.endsWith('_fail_if') // fail_if triggered
);
});
Expand Down Expand Up @@ -1112,6 +1113,7 @@
return (
ruleId.endsWith('/error') ||
ruleId.includes('/execution_error') ||
ruleId.includes('timeout') ||
ruleId.endsWith('_fail_if')
);
});
Expand Down Expand Up @@ -2912,6 +2914,7 @@
* Fatal issues are those indicating the check itself failed to execute properly:
* - ruleId ends with '/error' (system errors, exceptions)
* - ruleId contains '/execution_error' (command failures)
* - ruleId contains 'timeout' (timeouts)
* - ruleId ends with '_fail_if' (fail_if condition triggered)
*
* Regular error/critical severity issues (e.g., security vulnerabilities found in code)
Expand All @@ -2928,6 +2931,7 @@
return (
ruleId.endsWith('/error') || // System errors
ruleId.includes('/execution_error') || // Command failures
ruleId.includes('timeout') || // Timeouts
(ruleId.endsWith('_fail_if') && ruleId !== 'global_fail_if') // check-level fail_if only
);
});
Expand Down Expand Up @@ -2995,6 +2999,7 @@
return (
ruleId.endsWith('/error') || // System errors, exceptions
ruleId.includes('/execution_error') || // Command failures
ruleId.includes('timeout') || // Timeouts
(ruleId.endsWith('_fail_if') && ruleId !== 'global_fail_if') // check-level fail_if only
);
});
Expand Down Expand Up @@ -3129,7 +3134,7 @@
}

// Use extended Liquid with our custom filters/tags
const liquid = createExtendedLiquid({

Check warning on line 3137 in src/state-machine/states/level-dispatch.ts

View check run for this annotation

probelabs / Visor: performance

performance Issue

Creating new Liquid instance for each template rendering call is inefficient
Raw output
Cache the createExtendedLiquid() instance at module level to avoid recreating it for each template rendering.
trimTagLeft: false,
trimTagRight: false,
trimOutputLeft: false,
Expand Down
Loading
Loading