Skip to content

Conversation

@buger
Copy link
Contributor

@buger buger commented Jan 30, 2026

Summary

  • Fixed the readfile Liquid tag not being available when rendering parameters passed to remote workflows
  • Ensure command timeouts are treated as execution failures and surfaced to Slack when a check completes
  • Refresh existing worktrees when a ref advances to avoid stale branch checkouts
  • Require explicit mentions in channels (public C* and private G*); keep 1:1 DMs auto-triggered

Changes

  • Use createExtendedLiquid() in WorkflowCheckProvider constructor
  • Use createExtendedLiquid() in WorkflowExecutor constructor
  • Use createExtendedLiquid() in test-runner output computation
  • Added test case to verify readfile tag works in workflow args
  • Detect timeout errors in command provider and classify them as execution failures
  • Post Slack error notices for execution failures on completed checks
  • Update routing/stats to treat timeouts as fatal execution failures
  • Refresh worktree commits when reusing an existing worktree and the ref moved
  • Added tests for timeout handling and worktree refresh
  • Treat only DMs (D*) as DM-like; channels (C* and G*) now require explicit mention
  • Added gating test for G* channels

Root Cause

The readfile tag is registered via configureLiquidWithExtensions() in src/liquid-extensions.ts. However, when workflow parameters are rendered (in the prepareInputs method), the code was using a plain new Liquid() instance that doesn't have the custom extensions registered.

Test Plan

  • npx jest tests/frontends/slack-frontend.test.ts tests/unit/command-check-failure-detection.test.ts
  • npx jest tests/unit/worktree-manager.test.ts
  • npx jest tests/integration/slack-socket-gating.test.ts
  • npm run format:check

Fixes #305

The `readfile` liquid tag registered in `liquid-extensions.ts` was not
available when rendering parameters passed to remote workflows. This was
because `WorkflowCheckProvider`, `WorkflowExecutor`, and the test runner
were creating plain `Liquid` instances instead of using the extended
Liquid instance with custom tags and filters.

Changes:
- Use `createExtendedLiquid()` in WorkflowCheckProvider constructor
- Use `createExtendedLiquid()` in WorkflowExecutor constructor
- Use `createExtendedLiquid()` in test-runner output computation
- Add test case to verify readfile tag works in workflow args

Fixes #305

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@probelabs
Copy link
Contributor

probelabs bot commented Jan 30, 2026

PR Overview: Enable readfile Liquid Tag in Workflow Args

Summary

This PR fixes a critical bug where the {% readfile %} Liquid tag was not available when rendering parameters passed to remote workflows. The root cause was that workflow parameter rendering used a plain new Liquid() instance instead of the extended Liquid engine with custom tags registered via createExtendedLiquid().

Key Changes

1. Enable readfile Tag in Workflow Context (Primary Fix)

Root Cause: The readfile tag is registered in src/liquid-extensions.ts via configureLiquidWithExtensions(), but workflow parameter rendering was using new Liquid() which doesn't include custom extensions.

Files Changed:

  • src/providers/workflow-check-provider.ts: Changed constructor to use createExtendedLiquid() instead of new Liquid()
  • src/workflow-executor.ts: Changed constructor to use createExtendedLiquid() instead of new Liquid()
  • src/test-runner/index.ts: Changed output computation to use createExtendedLiquid() for workflow output templates

Impact: All workflow executions now have access to:

  • {% readfile %} tag for reading file contents
  • Custom filters: parse_json, to_json, base64, base64_decode, safe_label, shell_escape, etc.
  • Permission filters: has_min_permission, is_owner, is_member, etc.
  • Memory filters: memory_get, memory_has, memory_list

2. Timeout Error Handling & Classification

Problem: Command timeouts were not consistently treated as execution failures, causing dependent checks to run when they shouldn't.

Files Changed:

  • src/providers/command-check-provider.ts: Added fallback timeout detection based on error message text (/timed out/i)
  • src/state-machine/dispatch/stats-manager.ts: Added ruleId.includes('timeout') to fatal issue detection
  • src/state-machine/states/level-dispatch.ts: Updated hasFatalIssues() to include timeout ruleIds in multiple locations
  • src/state-machine/states/routing.ts: Enhanced classifyFailure() to detect timeouts via ruleId and message patterns

Impact: Timeouts now consistently:

  • Are classified as execution failures (not logical failures)
  • Skip dependent checks via fail-fast logic
  • Trigger Slack error notifications

3. Slack Error Notifications for Execution Failures

Problem: Execution failures (including timeouts) were not surfaced to Slack when checks completed.

Files Changed:

  • src/frontends/slack-frontend.ts:
    • Added maybePostExecutionFailure() method to post error notices for execution failures
    • Added isExecutionFailureIssue() helper to detect timeout and execution error issues
    • Moved telemetry trace_id logging behind telemetry check

Impact: Users now receive Slack notifications when:

  • Commands timeout
  • Execution errors occur
  • Checks fail with fatal issues

4. Worktree Refresh on Ref Advance

Problem: Reusing existing worktrees could result in stale checkouts if the branch advanced.

Files Changed:

  • src/utils/worktree-manager.ts:
    • Added logic to detect when ref has advanced (commit SHA changed)
    • Fetches latest commit and checks out new SHA when reusing worktree
    • Improved fetchRef() to return boolean success status

Impact: Long-running processes that reuse worktrees now get fresh code when branches advance.

5. Workflow Import Improvements

Problem: Workflow imports lacked circular dependency detection and proper error handling.

Files Changed:

  • src/workflow-registry.ts:
    • Added recursive import handling with visited Set for cycle detection
    • Support for both top-level and workflow-level imports
    • Enhanced loadWorkflowContent() to return resolved source and import base path
  • src/config.ts: Silent handling of 'already exists' errors for duplicate workflow imports

Impact: More robust workflow import system with:

  • Circular dependency prevention
  • Better error messages
  • Support for nested imports

6. Bot Message Filtering Changes

Problem: Overly restrictive bot filtering prevented legitimate third-party bot integrations.

Files Changed:

  • src/index.ts: Changed to only skip Visor's own bots (visor[bot], github-actions[bot])
  • src/slack/socket-runner.ts:
    • Enhanced filtering logic with debug logging
    • Explicit bot mention detection
    • Treat only 1:1 DMs (D*) as DM-like; private channels (G*) require explicit mention

Impact: Third-party bots can now trigger Visor for integration purposes while preventing loops.

Architecture & Impact

graph TD
    A[WorkflowCheckProvider] -->|createExtendedLiquid| B[Liquid Engine]
    C[WorkflowExecutor] -->|createExtendedLiquid| B
    D[TestRunner] -->|createExtendedLiquid| B
    B -->|configureLiquidWithExtensions| E[readfile Tag]
    B -->|configureLiquidWithExtensions| F[Custom Filters]
    E -->|Read files| G[File System]
    
    H[CommandCheckProvider] -->|Detect timeout| I[Error Classifier]
    I -->|Classify as fatal| J[StatsManager]
    J -->|Skip dependents| K[LevelDispatch]
    
    L[SlackFrontend] -->|Check completed| M[Execution Failure Detector]
    M -->|Post error| N[Slack API]
    
    O[WorktreeManager] -->|Reuse worktree| P[Check ref advance]
    P -->|Ref moved| Q[Checkout new commit]
Loading

Component Relationships:

  • Liquid Extensions: Centralized in src/liquid-extensions.ts with createExtendedLiquid() factory
  • Workflow Pipeline: WorkflowCheckProvider → WorkflowExecutor → Liquid rendering with full tag support
  • Error Handling: Command timeout detection → Stats classification → Routing decisions → Slack notifications
  • Worktree Management: Reuse with refresh logic prevents stale checkouts

Scope Discovery & Context Expansion

Related Files:

  • src/liquid-extensions.ts - Core extension registration (defines createExtendedLiquid() and ReadFileTag)
  • src/types/workflow.ts - Workflow definition types
  • src/providers/check-provider.ts - Base provider class
  • src/state-machine/states/*.ts - State machine logic
  • docs/liquid-templates.md - Liquid template documentation
  • docs/workflows.md - Workflow system docs

Impact Boundaries:

  • Workflow System: All workflow executions now have access to readfile tag and custom filters
  • Error Handling: Timeouts now consistently treated as fatal failures across the system
  • Bot Integration: More permissive bot filtering enables third-party integrations
  • Git Operations: Worktree refresh prevents stale code checkouts in long-running processes

Testing Coverage

New tests added:

  • tests/unit/workflow-check-provider.test.ts: readfile tag functionality in workflow args
  • tests/unit/command-check-failure-detection.test.ts: Timeout handling and dependent check skipping
  • tests/unit/worktree-manager.test.ts: Worktree refresh when ref advances
  • tests/unit/workflow-registry.test.ts: Recursive imports with cycle detection
  • tests/frontends/slack-frontend.test.ts: Execution failure Slack notifications
  • tests/integration/slack-socket-gating.test.ts: Bot message handling with explicit mentions

Review Notes

Key areas to focus on:

  1. Security: readfile tag path traversal protection (already implemented in liquid-extensions.ts with path validation)
  2. Performance: Worktree refresh logic impact on long-running workflows
  3. Bot filtering: Security implications of allowing third-party bots (now requires explicit mentions in private channels)
  4. Error handling: Consistency of timeout classification across components

Potential concerns:

  • SSRF protection for workflow imports from URLs (noted in review comments)
  • Duplicate 'already exists' error handling across multiple files (now consistent)
  • Empty catch blocks in SlackSocketRunner (debug logging failures)
Metadata
  • Review Effort: 3 / 5
  • Primary Label: bug

Powered by Visor from Probelabs

Last updated: 2026-01-30T17:23:07.789Z | Triggered by: pr_updated | Commit: 6ee44af

💡 TIP: You can chat with Visor using /visor ask <your question>

@probelabs
Copy link
Contributor

probelabs bot commented Jan 30, 2026

Security Issues (8)

Severity Location Issue
🟠 Error src/workflow-registry.ts:415
Workflow imports from URLs lack SSRF protection. The loadWorkflowContent() method fetches workflows from HTTP/HTTPS URLs without any validation against an allowlist or SSRF prevention controls. Unlike config-loader.ts which has validateRemoteURL() with allowlist checking, workflow imports have no such protection.
💡 SuggestionAdd URL validation similar to config-loader.ts validateRemoteURL() method. Implement an allowlist pattern check for workflow import URLs and add timeout controls to prevent SSRF attacks against internal services.
🟠 Error src/workflow-registry.ts:415
The fetch() calls for workflow imports (lines 424 and 431) lack timeout controls and User-Agent headers, making them vulnerable to slowloris attacks and potentially exposing internal service endpoints.
💡 SuggestionAdd AbortController with timeout and User-Agent header to all fetch() calls: const controller = new AbortController(); setTimeout(() => controller.abort(), 30000); await fetch(url, { signal: controller.signal, headers: { 'User-Agent': 'Visor/1.0' } });
🟢 Info src/utils/worktree-manager.ts:377
The error message at line 377 includes the full stderr from git checkout command which could potentially expose sensitive information about the repository structure or file system paths in logs.
💡 SuggestionSanitize error messages before logging or sending to external systems. Consider logging the full error details only at debug level while using sanitized messages for user-facing output.
🟢 Info src/utils/worktree-manager.ts:549
The fetchRef() method now returns boolean but the error logging at line 549 includes stderr which could contain malicious content if the ref parameter was crafted to inject commands into error messages.
💡 SuggestionThe validateRef() method at line 827 provides good protection against command injection. Ensure all error messages that include user-controlled input are properly sanitized before logging.
🟢 Info src/config.ts:553
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.
💡 SuggestionExtract the duplicate 'already exists' error handling into a shared utility function to ensure consistent behavior across all workflow import locations.
🟡 Warning src/slack/socket-runner.ts:191
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.
💡 SuggestionImplement 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.
🟡 Warning src/slack/socket-runner.ts:191
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.
💡 SuggestionAdd logging to the catch block: } catch (error) { logger.warn('[SlackSocket] Failed to ensure client for botUserId detection:', error); }
🟡 Warning src/frontends/slack-frontend.ts:324
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.
💡 SuggestionAdd logging to ensure errors are not silently swallowed: } catch (error) { logger.error('[SlackFrontend] Failed to post execution failure:', error); }

Architecture Issues (7)

Severity Location Issue
🟢 Info src/slack/socket-runner.ts:168
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.
💡 SuggestionConsider 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.
🟢 Info src/providers/workflow-check-provider.ts:30
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.
💡 SuggestionConduct 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.
🟡 Warning src/workflow-registry.ts:144
Duplicate 'already exists' error handling logic appears in both workflow-registry.ts and config.ts. The same pattern of checking if all errors contain 'already exists' is duplicated across two files, creating maintenance burden and potential for inconsistencies.
💡 SuggestionExtract the duplicate 'already exists' error detection logic into a shared utility function that can be reused by both WorkflowRegistry.importInternal() and ConfigManager.loadConfigFromObject(). This would centralize the business logic for handling duplicate workflow imports.
🟡 Warning src/frontends/slack-frontend.ts:294
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.
💡 SuggestionCentralize 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.
🟡 Warning src/state-machine/states/routing.ts:81
The classifyFailure() function in routing.ts implements different timeout detection patterns than isExecutionFailureIssue() in slack-frontend.ts. routing.ts checks msgLower.includes('timed out') while slack-frontend.ts checks msgLower.includes('timed out') AND msg.includes('Command execution failed'). This inconsistency could lead to different failure classifications for the same issue.
💡 SuggestionUnify failure classification logic by having both routing.ts and slack-frontend.ts use a shared utility function for detecting execution failures and timeouts. This ensures consistent behavior across the system.
🟡 Warning src/workflow-registry.ts:144
The importInternal() method handles both top-level imports and workflow-level imports in the same function, creating nested loops and complex control flow. The function processes topImports, then iterates workflows, then processes workflowImports, making it difficult to follow and maintain.
💡 SuggestionConsider refactoring import logic to separate top-level import processing from workflow-level import processing. This could be achieved by extracting workflow-level import handling into a separate method or by restructuring the control flow to be more linear and easier to understand.
🟡 Warning src/utils/worktree-manager.ts:328
The worktree refresh logic is embedded within the existing worktree reuse path, creating a deeply nested control structure with try-catch blocks and conditional logic. The refresh operation (fetch, checkout, metadata update) is intertwined with the reuse logic, making it difficult to test and maintain independently.
💡 SuggestionExtract the worktree refresh logic into a separate method (e.g., refreshWorktreeIfAdvanced()) that can be called from the reuse path. This would improve testability, reduce nesting, and make the refresh operation more explicit and easier to understand.

Performance Issues (11)

Severity Location Issue
🟢 Info src/providers/workflow-check-provider.ts:30
Creating new Liquid instance with createExtendedLiquid() in constructor is efficient, but ensure the instance is reused across multiple executions rather than recreated
💡 SuggestionThe current implementation correctly reuses the instance. This is informational - the pattern is correct.
🟢 Info src/workflow-executor.ts:64
Creating new Liquid instance with createExtendedLiquid() in constructor is efficient, but ensure the instance is reused across multiple executions rather than recreated
💡 SuggestionThe current implementation correctly reuses the instance. This is informational - the pattern is correct.
🟢 Info src/slack/socket-runner.ts:171
Empty catch block after ensureClient() could hide errors and cause performance issues if client initialization fails repeatedly
💡 SuggestionAdd logging to the catch block to track client initialization failures. This helps diagnose performance issues from repeated failed attempts.
🟢 Info src/utils/worktree-manager.ts:544
Git fetch operation has 60 second timeout which may be too long for slow network conditions, causing delays
💡 SuggestionConsider making the timeout configurable or reducing it for better responsiveness. The current 60s timeout is reasonable for most cases but could be optimized.
🟡 Warning src/workflow-registry.ts:145
Recursive workflow imports without depth limit could cause stack overflow or excessive memory usage for deeply nested import chains
💡 SuggestionAdd a maximum depth limit to the importInternal method to prevent runaway recursion. Track depth in the visited Set or use a separate depth counter.
🟡 Warning src/utils/worktree-manager.ts:328
Synchronous git operations in worktree refresh logic could block event loop, especially for large repositories or slow network connections
💡 SuggestionConsider making git operations async or adding timeouts to prevent blocking. The fetchRef and checkout operations could benefit from Promise.race with timeout.
🟡 Warning src/test-runner/index.ts:544
Creating new Liquid instance for each output computation in test runner is inefficient for repeated calls
💡 SuggestionCache the createExtendedLiquid() instance at class level or module level to avoid recreating it for each output computation.
🟡 Warning src/state-machine/dispatch/on-init-handlers.ts:52
Creating new Liquid instance for each template rendering call in renderTemplateArguments is inefficient
💡 SuggestionCache the createExtendedLiquid() instance at module level or pass it as a parameter to avoid recreating it for each template rendering.
🟡 Warning src/state-machine/dispatch/template-renderer.ts:53
Creating new Liquid instance for each template rendering call is inefficient
💡 SuggestionCache the createExtendedLiquid() instance at module level to avoid recreating it for each template rendering.
🟡 Warning src/state-machine/states/level-dispatch.ts:3137
Creating new Liquid instance for each template rendering call is inefficient
💡 SuggestionCache the createExtendedLiquid() instance at module level to avoid recreating it for each template rendering.
🟡 Warning src/workflow-registry.ts:137
Recursive workflow imports with URL fetching could cause multiple network requests in parallel, potentially overwhelming the network
💡 SuggestionConsider adding request throttling or concurrency limits when importing multiple workflows from URLs.

Quality Issues (17)

Severity Location Issue
🟠 Error src/slack/socket-runner.ts:223
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.
💡 SuggestionAdd debug logging to all catch blocks: catch (error) { if (process.env.VISOR_DEBUG === 'true') { logger.debug('[SlackSocket] Error in X:', error); } }
🟠 Error src/frontends/slack-frontend.ts:327
Empty catch block in maybePostExecutionFailure silently suppresses all errors. This could prevent critical error notifications from being posted.
💡 SuggestionAdd error logging: catch (error) { logger.error('[SlackFrontend] Failed to post execution failure:', error); }
🟠 Error src/providers/command-check-provider.ts:1148
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.
💡 SuggestionExtract logical sections into separate methods: renderCommandTemplate(), parseCommandOutput(), applyTransforms(), extractIssues(), handleExecutionError().
🟠 Error src/state-machine/dispatch/stats-manager.ts:12
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.
💡 SuggestionConsolidate into a single shared utility function in a common module: src/state-machine/utils/fatal-issues.ts
🟠 Error src/providers/workflow-check-provider.ts:796
The 'already exists' error handling logic is duplicated from config.ts and workflow-registry.ts. This is the third instance of the same pattern.
💡 SuggestionURGENT: Extract to shared utility function to prevent further duplication. The pattern has been copied 3 times in this PR alone.
🟡 Warning tests/unit/command-check-failure-detection.test.ts:50
Test uses magic number 10000 for setTimeout and 50 for timeout without clear semantic meaning. These values appear arbitrary and may need adjustment if system performance changes.
💡 SuggestionDefine named constants at the top of the test file: const LONG_RUNNING_COMMAND_MS = 10000; const SHORT_TIMEOUT_MS = 50; This makes the test intent clearer and easier to maintain.
🟡 Warning tests/unit/worktree-manager.test.ts:145
Test uses magic number 60000 for git command timeout without explanation. This value appears to be a copy-paste from production code rather than a deliberate test choice.
💡 SuggestionUse a named constant like GIT_COMMAND_TIMEOUT_MS = 60000 or explain why this specific timeout value is important for the test.
🟡 Warning tests/frontends/slack-frontend.test.ts:100
Test uses magic number 1000 for timeout in error message assertion. This value is derived from implementation details rather than requirements.
💡 SuggestionExtract the timeout value to a named constant or derive it from the test configuration to make the test more maintainable.
🟡 Warning src/slack/socket-runner.ts:195
Empty catch block after ensureClient() call silently suppresses errors. This could hide initialization problems and make debugging difficult.
💡 SuggestionAdd logging to the catch block: catch (error) { logger.debug('[SlackSocket] Failed to ensure client:', error); }
🟡 Warning src/workflow-registry.ts:145
The 'already exists' error handling logic is duplicated in workflow-registry.ts, config.ts, and workflow-check-provider.ts. This creates maintenance burden and risk of inconsistencies.
💡 SuggestionExtract the duplicate import error handling into a shared utility function: function handleImportDuplicateError(source: string, errors: ValidationError[]): void { ... }
🟡 Warning src/state-machine/states/routing.ts:95
Timeout detection uses multiple string matching patterns (ruleId.includes('timeout'), msgLower.includes('timed out')) which is fragile and could miss edge cases or produce false positives.
💡 SuggestionCreate a centralized timeout detection utility: function isTimeoutIssue(issue: ReviewIssue): boolean { const ruleId = String(issue.ruleId); const msg = String(issue.message || '').toLowerCase(); return ruleId.includes('timeout') || msg.includes('timed out') || msg.includes('timeout'); }
🟡 Warning src/utils/worktree-manager.ts:328
Broad catch block around worktree refresh logic logs a warning but continues with potentially stale worktree. This could cause checks to run on outdated code.
💡 SuggestionConsider rethrowing critical errors or adding a flag to track refresh failures: catch (error) { logger.warn(`Failed to refresh worktree: ${error}`); refreshFailed = true; }
🟡 Warning src/workflow-registry.ts:140
The importInternal method uses recursion with a visited Set for cycle detection, but the recursion depth could cause stack overflow for deeply nested import chains.
💡 SuggestionConsider converting to iterative approach using a stack/queue: const stack = [{source, options}]; while (stack.length > 0) { const current = stack.pop(); ... }
🟡 Warning tests/integration/slack-socket-gating.test.ts:56
Test for private channel (G*) gating lacks negative test cases for edge conditions: empty botUserId, malformed channel IDs, or mixed channel types in same test suite.
💡 SuggestionAdd test cases for: 1) Bot message without botUserId set, 2) Invalid channel format, 3) Channel type changes during execution, 4) Concurrent messages from different channel types.
🟡 Warning tests/unit/workflow-check-provider.test.ts:247
Test for readfile tag only tests success path. Missing negative tests for: non-existent files, permission errors, path traversal attempts, or invalid file paths.
💡 SuggestionAdd test cases for error conditions: 1) Reading non-existent file, 2) Path traversal attempts (../etc/passwd), 3) Permission denied errors, 4) Binary file handling, 5) Symlink handling.
🟡 Warning src/frontends/slack-frontend.ts:294
The isExecutionFailureIssue method uses multiple string matching patterns that are duplicated across the codebase (routing.ts has similar logic). This creates maintenance burden.
💡 SuggestionExtract to a shared utility: src/utils/issue-classifiers.ts with functions like isExecutionFailureIssue(), isTimeoutIssue(), isLogicalFailureIssue().
🟡 Warning src/config.ts:553
Silent skipping of 'already exists' errors during workflow import could hide real problems (e.g., partial imports, validation failures masked by duplicate detection).
💡 SuggestionAdd a counter or metric to track skipped imports, and log a warning if too many are skipped: logger.warn(`Skipped ${skippedCount} duplicate workflow imports`);

Powered by Visor from Probelabs

Last updated: 2026-01-30T17:23:11.429Z | Triggered by: pr_updated | Commit: 6ee44af

💡 TIP: You can chat with Visor using /visor ask <your question>

buger and others added 8 commits January 30, 2026 11:48
Previously, Visor blocked ALL bot messages to prevent recursion loops.
This was too restrictive and prevented integration with other bots.

Changes:
- GitHub: Remove `comment.user?.type === 'Bot'` check that blocked all bots
- Slack: Remove `if (ev.bot_id) return` check that blocked all bots
- Slack frontend: Remove `ev?.subtype === 'bot_message'` check

Visor still protects against self-loops by checking:
- Messages from visor[bot] and github-actions[bot]
- Comments with visor-comment-id markers
- Slack messages from Visor's own bot user ID

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Previously, when two workflow files both imported the same dependency,
Visor would throw a "Workflow already exists" error on the second import.

Now, duplicate imports are handled gracefully by skipping workflows that
are already registered. This allows multiple workflow files to declare
their imports for standalone testing while still working correctly when
composed together.

Changes:
- config.ts: Skip already-imported workflows in loadWorkflows()
- workflow-check-provider.ts: Simplify import handling to use same pattern

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

readfile liquid tag not available when passing parameters to remote workflows

2 participants