Refactor workflow activation logic and improve error handling

[Gkrumbach07]

Summary\n- remove redundant workflow activation in backend session handler\n- add retry logic with exponential backoff in workflow activation hook\n- avoid duplicate workflow updates by checking current state\n- improve logging and error handling around workflow activation\n\n## Testing\n- not run (not requested)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Review posted via separate comment

[github-actions]

Claude Code Review

Summary

This PR refactors workflow activation logic to improve reliability and prevent duplicate operations. The changes move workflow activation responsibility from the backend to the operator/runner, add retry logic with exponential backoff in the frontend, and implement proper state checking to avoid redundant workflow updates.

Issues by Severity

Critical Issues

1. Missing Error Response Field for Retry Logic

• Location: components/backend/handlers/sessions.go:1210-1256 (removed code), frontend expects errorData.retryable
• Issue: The frontend retry logic at use-workflow-management.ts:76 checks for errorData.retryable, but there is no evidence the backend sets this field when the runner is not ready. Without this field, retries will not trigger even when appropriate.
• Impact: Users may experience hard failures instead of automatic retries when the runner pod is still initializing.
• Recommendation: The backend handler should return {"error": "...", "retryable": true} when receiving 503 or connection errors from the runner.

2. Race Condition in Operator Reconciliation

• Location: components/operator/internal/handlers/sessions.go:1662-1670
• Issue: The operator checks reconciledPath in addition to reconciledGitURL and reconciledBranch, but the runner lock (_workflow_change_lock) does not prevent race conditions between the operator reconciliation and the runner state check.
• Impact: If the operator reconciles the workflow while the runner is checking/updating environment variables, duplicate workflow activations or missed updates could occur.
• Recommendation: The runner should read from the CR status field instead of environment variables to get the current state, or the operator should use a status condition to signal workflow reconciliation in progress.

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

[github-actions]

Major Issues

3. Input Validation Gaps in Runner

• Location: components/runners/claude-code-runner/main.py:516-518
• Issue: While the code strips whitespace from inputs, there is no validation for:
  • Valid Git URL format
  • Branch name format (e.g., no special characters that could cause command injection)
  • Path traversal attempts in the path field
• Pattern Violation: Security standards require input validation (see CLAUDE.md lines 95-109)
• Recommendation: Add validation using urllib.parse for URL validation and sanitize branch/path inputs before passing to git commands.

4. Error Handling Pattern Violation in Frontend

• Location: components/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-workflow-management.ts:96-104
• Issue: The finally block was removed, replaced with explicit setWorkflowActivating(false) calls. This creates inconsistency:
  • Success path: Sets to false at line 96
  • Error path: Sets to false at line 102
  • Missing: No cleanup if an exception is thrown before reaching error handler
• Pattern Violation: React Query patterns recommend using finally for cleanup to ensure state is always reset
• Recommendation: Restore the finally block to ensure workflowActivating is always reset, or wrap in try/finally at the function level.

5. Unbounded Retry Delay Accumulation

• Location: components/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-workflow-management.ts:77
• Issue: The exponential backoff calculation is correct, but there is no total timeout. If the backend keeps returning retryable: true, the function could retry for ~18 seconds plus network time, blocking the UI.
• Recommendation: Add an absolute timeout (e.g., 20s total) or consider moving retries to the backend/operator level where they do not block the UI thread.

[github-actions]

Minor Issues

6. Inconsistent Logging Levels

• Location: components/runners/claude-code-runner/main.py:532
• Issue: "Workflow unchanged" is logged as INFO but represents a no-op. Should be DEBUG to reduce log noise in production.
• Recommendation: Change to logger.debug("Workflow unchanged; skipping reinit and greeting").

7. Missing Type Annotations

• Location: components/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-workflow-management.ts:35
• Issue: The retryCount parameter lacks explicit type annotation in the callback signature.
• Pattern Violation: Frontend standards require explicit types (CLAUDE.md line 1115)
• Recommendation: Change to retryCount: number = 0 for clarity.

8. Magic Numbers Without Constants

• Location: use-workflow-management.ts:77 and :76
• Issue: Retry configuration is hardcoded with magic numbers (1000, 1.5, 5000, 5)
• Recommendation: Extract to named constants

9. Operator Path Comparison Edge Case

• Location: components/operator/internal/handlers/sessions.go:1665
• Issue: The path comparison does not normalize paths (e.g., "./foo" vs "foo", trailing slashes). While unlikely to cause issues, it could lead to unnecessary re-clones.
• Recommendation: Normalize paths before comparison using path.Clean() (Go standard library).

[github-actions]

Positive Highlights

1. Excellent Removal of Redundant Code

• The backend direct runner HTTP call (39 lines removed) was indeed redundant since the operator already handles workflow reconciliation. This follows the Single Responsibility Principle perfectly.

2. Proper Concurrency Control

• The _workflow_change_lock in main.py:522 prevents race conditions from concurrent workflow change requests. This is a best practice for async Python code.

3. Smart Idempotency Check

• The state comparison at main.py:527-533 prevents unnecessary workflow re-clones and greeting messages, reducing unnecessary load and improving UX.

4. Appropriate Retry Strategy

• Exponential backoff with max cap (1.5x factor, 5s max) is a textbook approach for handling transient failures. The 5-attempt limit prevents infinite loops.

5. State Management Improvements

• Only setting workflowActivating on the first attempt (retryCount === 0) prevents UI flicker during retries. This is a thoughtful UX enhancement.

6. Better Separation of Concerns

• Moving workflow activation to the operator/runner aligns with the architecture where the operator is the reconciliation controller. The backend now only updates the CR, which is its proper role.

[github-actions]

Recommendations

High Priority (Before Merge)

1. Add retryable field to backend error responses (Critical Issue Outcome: Reduce Refinement Time with agent System #1)

   • When the backend receives errors from the runner, check if they are retryable (503, connection refused, timeout) and include "retryable": true in the error response

2. Add input validation to runner (Major Issue Epic: Data Source Integration #3)

   • Validate Git URLs
   • Validate branch name format
   • Validate path for traversal attempts

3. Restore finally block in frontend (Major Issue Epic: AI Agent Development #4)

   • Wrap the try-catch in activateWorkflow with a finally block to ensure cleanup
   • Or add explicit cleanup at all exit points

Medium Priority (Consider for Follow-up)

4. Add absolute timeout for retries (Major Issue Epic: Jira Integration & Workflow #5)
5. Fix operator race condition (Critical Issue Epic: RAT Architecture & Design #2)
6. Extract magic numbers to constants (Minor Issue Test: Updated Workflow Validation #8)

Low Priority (Nice-to-Have)

7. Adjust log levels (Minor Issue Epic: Testing & Validation #6)
8. Add type annotations (Minor Issue Test: Automation Workflow Validation #7)
9. Normalize paths in operator (Minor Issue Bump actions/add-to-project from 0.5.0 to 1.0.2 #9)

[github-actions]

Architecture Compliance

Kubernetes Client Usage: PASS

• Backend correctly uses user-scoped clients (removed code used reqDyn properly)
• Operator uses service account client appropriately

Error Handling: PARTIAL

• Backend follows patterns (removed code had proper error handling)
• Frontend retry logic is good but missing finally block
• Runner needs validation errors

Security: NEEDS IMPROVEMENT

• Input validation missing in runner (see Critical Issue Epic: Data Source Integration #3)
• No evidence of token/secret logging issues

React Query Patterns: MOSTLY PASS

• Good use of retry logic
• Missing finally cleanup pattern (see Major Issue Epic: AI Agent Development #4)

Testing Recommendations

Since testing was not requested/run, here are key scenarios to verify:

1. Retry Logic: Simulate runner not ready (503) and verify frontend retries with proper backoff
2. Idempotency: Call workflow change endpoint twice with same params, verify only one clone/greeting
3. Race Condition: Rapidly change workflows, verify no duplicate activations or missed updates
4. Input Validation: Try malicious inputs (path traversal, shell injection in branch)
5. Operator Reconciliation: Manually edit CR activeWorkflow, verify operator detects drift including path changes

[github-actions]

Conclusion

This is a solid refactoring that improves architecture by removing redundant code and adding resilience through retry logic. The core logic is sound and follows established patterns well.

However, there are 3 critical/major issues that should be addressed before merge:

1. Missing retryable field in error responses - This blocks the retry logic from working as intended
2. Input validation gaps - Security concern that could allow malicious inputs
3. Missing finally block - Reliability concern that could leave UI in inconsistent state

With these addressed, this PR will be ready to merge. The positive improvements (idempotency, concurrency control, separation of concerns) outweigh the issues, making this a net positive change once the gaps are filled.

Recommendation: Request changes for items 1-3, then approve after fixes.