From c3e4ff94cdfe738eb560c5197658bf2f25ae46af Mon Sep 17 00:00:00 2001 From: Vikas Pandey Date: Mon, 13 Jul 2026 19:38:34 +0530 Subject: [PATCH 1/2] WG013: flag catching Throwable or Error in workflow code New forbidden-catch-type declarative rule type, alongside forbidden-method and mutable-side-effect-equality: flags catch (Throwable ...)/catch (Error ...) reachable from a workflow entry point, including a multi-catch's individual component types. The SDK uses unchecked Error subclasses internally for cancellation and workflow-task-failure control flow; a bare Throwable/Error catch swallows those signals along with everything else, so there's no legitimate reason for workflow code to catch either directly. A catch clause's caught type is neither a method call nor a constructor call, so no existing CallTarget can match it. CallGraphAnalyzer gains findCaughtTypeMatches, reusing the same path/visiting/context bookkeeping findCallPaths already has (recordMatch, effectiveContext, resolveToSource) without touching search() itself. TemporalRuleSupport.findViolations is refactored to split its filter-and-convert step into a reusable toViolations(List, ...), so ForbiddenCatchTypeRule shares the same suppressedContexts/requiredContexts handling ForbiddenMethodRule uses instead of a third copy of that logic. Adds docs/rules/WG013.md and CatchThrowableOrErrorRuleTest, covering direct catch, the Error variant, one-hop-through-a-helper, multi-catch component matching, still-firing inside Workflow.sideEffect() (unlike WG001's suppression), not flagging specific checked exceptions, and not flagging inside Activity implementations. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 16 + docs/rules/WG013.md | 126 +++++++ .../wogu/temporal/ForbiddenCatchTypeRule.java | 80 +++++ .../dev/wogu/temporal/RuleDefinition.java | 5 + .../wogu/temporal/RuleDefinitionLoader.java | 1 + .../java/dev/wogu/temporal/RuleRegistry.java | 3 +- .../wogu/temporal/TemporalRuleSupport.java | 42 ++- .../temporal/callgraph/CallGraphAnalyzer.java | 115 +++++++ .../src/main/resources/rules/wg013.yaml | 25 ++ .../CatchThrowableOrErrorRuleTest.java | 322 ++++++++++++++++++ .../MutableSideEffectEqualityRuleTest.java | 1 + .../temporal/RuleDefinitionLoaderTest.java | 55 ++- .../dev/wogu/temporal/RuleDefinitionTest.java | 1 + .../dev/wogu/temporal/RuleRegistryTest.java | 74 +++- .../TemporalWorkflowValidatorTest.java | 4 +- 15 files changed, 852 insertions(+), 18 deletions(-) create mode 100644 docs/rules/WG013.md create mode 100644 wogu-temporal/src/main/java/dev/wogu/temporal/ForbiddenCatchTypeRule.java create mode 100644 wogu-temporal/src/main/resources/rules/wg013.yaml create mode 100644 wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index f0fa2b9..2575034 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,22 @@ project adheres to [Semantic Versioning](https://semver.org/) (see ### Added +- **WG013 — Catching Throwable or Error in workflow code** (Determinism). Flags + `catch (Throwable ...)`/`catch (Error ...)` reachable from a workflow entry point, + including a multi-catch's individual component types: the SDK uses unchecked `Error` + subclasses internally for cancellation and workflow-task-failure control flow, and a + bare `Throwable`/`Error` catch swallows those signals along with everything else. A third + new declarative rule type, `forbidden-catch-type`, alongside `forbidden-method` and + `mutable-side-effect-equality`: a `catch` clause's caught type is neither a method call + nor a constructor call, so it's not something any existing `CallTarget` can match. + `CallGraphAnalyzer` gains `findCaughtTypeMatches`, a new traversal method reusing all of + `findCallPaths`'s existing path/visiting/context bookkeeping (`recordMatch`, + `effectiveContext`, `resolveToSource`) without touching `search()` itself. + `TemporalRuleSupport.findViolations` is refactored to split its filter-and-convert step + into a reusable `toViolations(List, ...)`, so `ForbiddenCatchTypeRule` + shares the exact same `suppressedContexts`/`requiredContexts` handling + `ForbiddenMethodRule` uses instead of a third copy of that logic. `docs/rules/WG013.md` + documents it. - **WG012 — MutableSideEffect with reference-equality value type** (Determinism). Flags `Workflow.mutableSideEffect(id, valueClass, updateFunction, func)` calls whose value type relies on inherited, identity-based `Object.equals()` — since `func` constructs a new diff --git a/docs/rules/WG013.md b/docs/rules/WG013.md new file mode 100644 index 0000000..e84500b --- /dev/null +++ b/docs/rules/WG013.md @@ -0,0 +1,126 @@ +# WG013 — Catching Throwable or Error in workflow code + +| | | +|---|---| +| **Rule ID** | WG013 | +| **Category** | Determinism ([WG001–WG099](../../CONTRIBUTING.md#rule-numbering)) | +| **Severity** | ERROR | +| **Engine** | Temporal Java SDK | +| **Since** | 1.0.0 | +| **Auto Fix** | Not Available | + +## Problem + +A Temporal workflow implementation — or any code reachable from a workflow's entry point, +however many method calls away — contains a `catch (Throwable ...)` or `catch (Error ...)` +clause. + +## Why this matters + +The Temporal Java SDK uses unchecked `Error` subclasses internally as control-flow signals: +for cancellation, and for the destroy path a workflow task takes when it fails +deterministically and needs to safely stop and retry. Both are implemented as `Error`, not +as a checked or ordinary runtime exception, specifically so application code's normal +`catch (Exception ...)` handlers don't accidentally intercept them. + +`catch (Throwable ...)` and `catch (Error ...)` don't respect that boundary — they catch +everything, including the SDK's own internal signals. A workflow that swallows them this way +can silently fail to propagate a cancellation, or can limp forward in a corrupted state after +a workflow task failure that was supposed to stop it cleanly. There is no legitimate reason +for workflow code to catch either type directly. + +## Bad Example + +```java +public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void processPayment(String accountId) { + try { + activities.chargeCard(accountId); + } catch (Throwable t) { + logger.warn("ignoring failure", t); // swallows Temporal's internal Error signals too + } + } +} +``` + +This is flagged even when the `catch` is several methods away from the workflow entry point: + +```java +public class PaymentWorkflowImpl implements PaymentWorkflow { + private final PaymentService paymentService = new PaymentService(); + + @Override + public void processPayment(String accountId) { + // WG013 still fires: WoGu's call-graph analysis follows this call into + // PaymentService.chargeCardSafely() and finds catch (Throwable) inside it. + paymentService.chargeCardSafely(accountId); + } +} + +class PaymentService { + void chargeCardSafely(String accountId) { + try { + activities.chargeCard(accountId); + } catch (Throwable t) { + // swallowed + } + } +} +``` + +A multi-catch is checked component by component, so `catch (IOException | Throwable t)` is +flagged for its `Throwable` component even though `IOException` alone would be fine. + +## Good Example + +```java +public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void processPayment(String accountId) { + try { + activities.chargeCard(accountId); + } catch (ActivityFailure e) { + // handle the specific, expected failure + } + } +} +``` + +## Recommended Fix + +Catch specific checked or application exceptions (`ActivityFailure`, your own business +exceptions) instead of `Throwable` or `Error`. If you need a catch-all for logging or +cleanup purposes, catch `Exception` — never `Throwable`, and never `Error`. + +## References + +- [Temporal documentation: Workflow determinism](https://docs.temporal.io/workflows#deterministic-constraints) + +## False Positives + +WG013 uses the same call-graph analysis as WG001–WG012: starting from a workflow's +entry-point method(s), it follows every method call that can be resolved to another +method's source within the same project, however many hops deep, looking for a `catch` +clause matching `java.lang.Throwable` or `java.lang.Error` along the way — including inside +a `Workflow.sideEffect(...)`/`Workflow.mutableSideEffect(...)` callback, since the danger +(swallowing an internal SDK signal on the workflow thread) applies there just as much as in +ordinary workflow code; unlike WG001, WG013 declares no `suppressedContexts`. + +As with every call-graph-based rule, this analysis intentionally stops, without reporting +anything past that point, when it reaches a call it cannot resolve to source it can see — +most notably, a call into an **Activity** implementation (reached through the Activity's +interface, which has no body to look inside, so the implementation's real `catch` clauses +are never reached), a call into a compiled dependency, or a call resolved only through +reflection or dynamic dispatch. + +This means WG013 can produce **false negatives** (a real violation hidden behind a call it +can't see into) but is designed to avoid **false positives** — it will not flag a `catch` +clause for any type other than `Throwable`/`Error` themselves; catching a subclass of +`Exception`, or any other checked/unchecked exception type, is untouched. If you believe +WG013 is flagging or missing something incorrectly, please open an issue with a minimal +reproduction. + +## Since Version + +1.0.0 diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/ForbiddenCatchTypeRule.java b/wogu-temporal/src/main/java/dev/wogu/temporal/ForbiddenCatchTypeRule.java new file mode 100644 index 0000000..ce15eee --- /dev/null +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/ForbiddenCatchTypeRule.java @@ -0,0 +1,80 @@ +package dev.wogu.temporal; + +import com.github.javaparser.ast.body.MethodDeclaration; +import dev.wogu.api.Rule; +import dev.wogu.api.ValidationContext; +import dev.wogu.api.Violation; +import dev.wogu.temporal.callgraph.CallGraphAnalyzer; +import dev.wogu.temporal.callgraph.CallGraphMatch; +import dev.wogu.temporal.callgraph.ContextEntryPoint; +import dev.wogu.temporal.callgraph.ExecutionContext; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; + +/** + * The {@code forbidden-catch-type} declarative rule type: flags a {@code catch} clause, + * reachable from a workflow entry point, whose caught type matches one of a + * {@link RuleDefinition}'s {@code catchTypes}. + * + *

This is the generic engine behind WG013 ({@code catch (Throwable)}/{@code catch + * (Error)}) and any future "flag catching this exception type" rule — none of which need a + * dedicated Java class. Adding one is purely a matter of adding a YAML definition with + * {@code type: forbidden-catch-type} and a {@code catchTypes} list under + * {@code src/main/resources/rules}; this class reads it, calls + * {@link CallGraphAnalyzer#findCaughtTypeMatches}, and reuses + * {@link TemporalRuleSupport#toViolations} for the same suppressed/required-context + * filtering and violation-building {@link ForbiddenMethodRule} uses for its own shape — the + * only real difference is which {@link CallGraphAnalyzer} method supplies the matches, + * since a {@code catch} clause's type isn't a method call or constructor a + * {@link dev.wogu.temporal.callgraph.CallTarget} can match. + */ +final class ForbiddenCatchTypeRule implements TemporalRule { + + private final Rule metadata; + private final List catchTypes; + private final String message; + private final String suggestedFix; + private final Set suppressedContexts; + private final Set requiredContexts; + + ForbiddenCatchTypeRule(RuleDefinition definition) { + this.metadata = definition.toRule(); + if (definition.catchTypes().isEmpty()) { + throw new IllegalArgumentException( + "Rule '" + definition.id() + "' of type 'forbidden-catch-type' requires at least one entry in " + + "'catchTypes'"); + } + this.catchTypes = definition.catchTypes(); + this.message = definition.description(); + this.suggestedFix = definition.replacement(); + this.suppressedContexts = + definition.suppressedContexts().stream().map(ForbiddenMethodRule::toExecutionContext).collect(Collectors.toUnmodifiableSet()); + this.requiredContexts = + definition.requiredContexts().stream().map(ForbiddenMethodRule::toExecutionContext).collect(Collectors.toUnmodifiableSet()); + } + + @Override + public Rule metadata() { + return metadata; + } + + @Override + public List evaluate( + ValidationContext context, + List workflowClasses, + CallGraphAnalyzer callGraph, + Predicate activityBoundary, + List contextEntryPoints) { + List matches = new ArrayList<>(); + for (ScannedWorkflowClass workflowClass : workflowClasses) { + for (MethodDeclaration entryPoint : workflowClass.entryPoints()) { + matches.addAll(callGraph.findCaughtTypeMatches(entryPoint, catchTypes, activityBoundary, contextEntryPoints)); + } + } + return TemporalRuleSupport.toViolations( + matches, metadata, context, message, suggestedFix, suppressedContexts, requiredContexts); + } +} diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinition.java b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinition.java index 025c13f..692f430 100644 --- a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinition.java +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinition.java @@ -48,6 +48,9 @@ * {@link dev.wogu.temporal.callgraph.ValueBasedEqualityArgumentTarget}): 0-based index * of the {@code Class} argument, on the single method named in {@link #methods()}, * whose resolved type must have real value-based equality; {@code null} for other types + * @param catchTypes for {@code forbidden-catch-type} rules: fully qualified names of + * exception types that must not be caught in code reachable from a workflow entry + * point, e.g. {@code "java.lang.Throwable"}; empty for other types * @param tags free-form labels, reserved for future use (not yet rendered anywhere) */ record RuleDefinition( @@ -66,6 +69,7 @@ record RuleDefinition( List suppressedContexts, List requiredContexts, Integer valueTypeArgumentIndex, + List catchTypes, List tags) { RuleDefinition { @@ -83,6 +87,7 @@ record RuleDefinition( constructors = List.copyOf(Objects.requireNonNull(constructors, "constructors")); suppressedContexts = List.copyOf(Objects.requireNonNull(suppressedContexts, "suppressedContexts")); requiredContexts = List.copyOf(Objects.requireNonNull(requiredContexts, "requiredContexts")); + catchTypes = List.copyOf(Objects.requireNonNull(catchTypes, "catchTypes")); tags = List.copyOf(Objects.requireNonNull(tags, "tags")); } diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java index 2c6f63e..e07df41 100644 --- a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java @@ -85,6 +85,7 @@ RuleDefinition parse(String yaml, String sourceName) { stringList(data, "suppressedContexts"), stringList(data, "requiredContexts"), optionalInt(data, "valueTypeArgumentIndex"), + stringList(data, "catchTypes"), stringList(data, "tags")); } diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleRegistry.java b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleRegistry.java index 7eb45c6..f4343e7 100644 --- a/wogu-temporal/src/main/java/dev/wogu/temporal/RuleRegistry.java +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/RuleRegistry.java @@ -24,7 +24,8 @@ final class RuleRegistry { private static final Map> FACTORIES_BY_TYPE = Map.of( "forbidden-method", ForbiddenMethodRule::new, - "mutable-side-effect-equality", MutableSideEffectEqualityRule::new); + "mutable-side-effect-equality", MutableSideEffectEqualityRule::new, + "forbidden-catch-type", ForbiddenCatchTypeRule::new); private RuleRegistry() {} diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/TemporalRuleSupport.java b/wogu-temporal/src/main/java/dev/wogu/temporal/TemporalRuleSupport.java index 7ed508d..99fe8bd 100644 --- a/wogu-temporal/src/main/java/dev/wogu/temporal/TemporalRuleSupport.java +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/TemporalRuleSupport.java @@ -98,22 +98,44 @@ static List findViolations( List contextEntryPoints, Set suppressedContexts, Set requiredContexts) { - List violations = new ArrayList<>(); + List matches = new ArrayList<>(); for (ScannedWorkflowClass workflowClass : workflowClasses) { for (MethodDeclaration entryPoint : workflowClass.entryPoints()) { for (CallTarget target : targets) { - for (CallGraphMatch match : callGraph.findCallPaths(entryPoint, target, activityBoundary, contextEntryPoints)) { - if (suppressedContexts.contains(match.executionContext())) { - continue; - } - if (!requiredContexts.isEmpty() && !requiredContexts.contains(match.executionContext())) { - continue; - } - violations.add(toViolation(rule, context, match, message, suggestedFix)); - } + matches.addAll(callGraph.findCallPaths(entryPoint, target, activityBoundary, contextEntryPoints)); } } } + return toViolations(matches, rule, context, message, suggestedFix, suppressedContexts, requiredContexts); + } + + /** + * Filters {@code matches} down to the ones {@code suppressedContexts}/{@code requiredContexts} + * allow, and converts what's left into {@link Violation}s. Split out from + * {@link #findViolations(List, CallGraphAnalyzer, List, Rule, ValidationContext, String, + * String, Predicate, List, Set, Set)} so a rule whose matches don't come from + * {@link CallGraphAnalyzer#findCallPaths} at all — e.g. {@link ForbiddenCatchTypeRule}'s + * {@link CallGraphAnalyzer#findCaughtTypeMatches} — can still reuse the same + * suppression/required-context semantics and violation shape instead of duplicating them. + */ + static List toViolations( + List matches, + Rule rule, + ValidationContext context, + String message, + String suggestedFix, + Set suppressedContexts, + Set requiredContexts) { + List violations = new ArrayList<>(); + for (CallGraphMatch match : matches) { + if (suppressedContexts.contains(match.executionContext())) { + continue; + } + if (!requiredContexts.isEmpty() && !requiredContexts.contains(match.executionContext())) { + continue; + } + violations.add(toViolation(rule, context, match, message, suggestedFix)); + } return violations; } diff --git a/wogu-temporal/src/main/java/dev/wogu/temporal/callgraph/CallGraphAnalyzer.java b/wogu-temporal/src/main/java/dev/wogu/temporal/callgraph/CallGraphAnalyzer.java index c3e991c..020cde7 100644 --- a/wogu-temporal/src/main/java/dev/wogu/temporal/callgraph/CallGraphAnalyzer.java +++ b/wogu-temporal/src/main/java/dev/wogu/temporal/callgraph/CallGraphAnalyzer.java @@ -7,6 +7,10 @@ import com.github.javaparser.ast.expr.MethodCallExpr; import com.github.javaparser.ast.expr.ObjectCreationExpr; import com.github.javaparser.ast.nodeTypes.NodeWithSimpleName; +import com.github.javaparser.ast.stmt.CatchClause; +import com.github.javaparser.ast.type.ClassOrInterfaceType; +import com.github.javaparser.ast.type.ReferenceType; +import com.github.javaparser.ast.type.Type; import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration; import com.github.javaparser.symbolsolver.javaparsermodel.declarations.JavaParserMethodDeclaration; import dev.wogu.api.CallPathFrame; @@ -124,6 +128,117 @@ public List findCallPaths( return results; } + /** + * Finds every {@code catch} clause reachable from {@code entryPoint} (however many + * method calls deep) whose caught type matches one of {@code caughtTypeNames} — checked + * the same way {@link ConstructorCallTarget} matches a constructor's type: an explicit + * import of the qualified name, a wildcard import of its package, no import needed for a + * {@code java.lang} type, or the fully qualified name written inline. A multi-catch + * (e.g. {@code catch (IOException | InterruptedException e)}) is checked component type + * by component type, so only the matching component is reported. + * + *

This exists alongside {@link #findCallPaths}, not as an overload of it, because a + * {@code catch} clause's type is neither a method call nor a constructor call — nothing + * {@link CallTarget} can match against — while everything else about "reachable from a + * workflow entry point, tracking {@link ExecutionContext}, however many hops deep" stays + * identical, so this reuses the same path/visiting/context bookkeeping. + * + * @param entryPoint the method to start traversal from + * @param caughtTypeNames fully qualified names of the types to flag when caught, e.g. + * {@code "java.lang.Throwable"} + * @param traversalBoundary methods this traversal must not enter + * @param contextEntryPoints calls that establish an {@link ExecutionContext} for their + * callback, the same as {@link #findCallPaths(MethodDeclaration, CallTarget, Predicate, List)} + * @return one match per matching {@code catch} clause found; empty if none are reachable + */ + public List findCaughtTypeMatches( + MethodDeclaration entryPoint, + List caughtTypeNames, + Predicate traversalBoundary, + List contextEntryPoints) { + List matchers = caughtTypeNames.stream().map(QualifiedClassNameMatcher::new).toList(); + List results = new ArrayList<>(); + Deque path = new ArrayDeque<>(); + path.addLast(frameFor(entryPoint, lineOf(entryPoint))); + Set visiting = Collections.newSetFromMap(new IdentityHashMap<>()); + collectCaughtTypeMatches( + entryPoint, matchers, path, visiting, results, 0, traversalBoundary, contextEntryPoints, ExecutionContext.NORMAL_WORKFLOW); + return results; + } + + private void collectCaughtTypeMatches( + MethodDeclaration method, + List matchers, + Deque path, + Set visiting, + List results, + int depth, + Predicate traversalBoundary, + List contextEntryPoints, + ExecutionContext inheritedContext) { + if (traversalBoundary.test(method)) { + return; + } + if (depth > MAX_DEPTH || !visiting.add(method)) { + return; + } + try { + method + .findCompilationUnit() + .ifPresent( + unit -> { + for (CatchClause catchClause : method.findAll(CatchClause.class)) { + ExecutionContext contextAtCatch = + effectiveContext(catchClause, method, unit, contextEntryPoints, inheritedContext); + for (ReferenceType caughtType : caughtTypesOf(catchClause)) { + matchingType(matchers, caughtType, unit) + .ifPresent(matcher -> recordMatch( + "catch (" + matcher.simpleClassName() + ")", + unit, + caughtType, + path, + results, + method, + contextAtCatch)); + } + } + for (MethodCallExpr call : method.findAll(MethodCallExpr.class)) { + ExecutionContext contextAtCall = + effectiveContext(call, method, unit, contextEntryPoints, inheritedContext); + resolveToSource(call).ifPresent(resolved -> { + path.addLast(frameFor(resolved, lineOf(call))); + collectCaughtTypeMatches( + resolved, matchers, path, visiting, results, depth + 1, traversalBoundary, contextEntryPoints, + contextAtCall); + path.removeLast(); + }); + } + }); + } finally { + visiting.remove(method); + } + } + + /** + * A {@code catch} clause's individual caught types: a single-element list for an + * ordinary catch, or one element per alternative for a multi-catch (e.g. + * {@code catch (IOException | InterruptedException e)}). + */ + private static List caughtTypesOf(CatchClause catchClause) { + Type type = catchClause.getParameter().getType(); + return type.isUnionType() ? type.asUnionType().getElements() : List.of((ReferenceType) type); + } + + private static Optional matchingType( + List matchers, ReferenceType caughtType, CompilationUnit unit) { + if (!(caughtType instanceof ClassOrInterfaceType classType)) { + return Optional.empty(); + } + String writtenSimpleName = classType.getScope().isPresent() ? "" : classType.getNameAsString(); + String writtenFullText = classType.toString(); + return matchers.stream().filter(matcher -> matcher.matches(writtenSimpleName, writtenFullText, unit)).findFirst(); + } + private void search( MethodDeclaration method, CallTarget target, diff --git a/wogu-temporal/src/main/resources/rules/wg013.yaml b/wogu-temporal/src/main/resources/rules/wg013.yaml new file mode 100644 index 0000000..0c3e529 --- /dev/null +++ b/wogu-temporal/src/main/resources/rules/wg013.yaml @@ -0,0 +1,25 @@ +id: WG013 +type: forbidden-catch-type +title: Catching Throwable or Error in workflow code +category: Determinism +severity: ERROR +engine: Temporal Java SDK +since: 1.0.0 +documentation: https://github.com/vikas0686/wogu/tree/main/docs/rules/WG013.md + +description: >- + The Temporal SDK uses unchecked Error subclasses internally as control-flow + signals, including for cancellation and for the destroy path when a workflow + task fails deterministically. Catching Throwable or Error in workflow code + swallows those internal signals along with everything else, so a cancellation + can silently fail to propagate, or a workflow task that should safely + fail-and-retry instead limps forward in a corrupted state. + +replacement: >- + Catch specific checked or application exceptions (ActivityFailure, your own + business exceptions) instead of Throwable or Error. There is no legitimate + reason for workflow code to catch either directly. + +catchTypes: + - java.lang.Throwable + - java.lang.Error diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java new file mode 100644 index 0000000..ad2b1e9 --- /dev/null +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java @@ -0,0 +1,322 @@ +package dev.wogu.temporal; + +import static org.assertj.core.api.Assertions.assertThat; + +import dev.wogu.api.CallPathFrame; +import dev.wogu.api.RuleResult; +import dev.wogu.api.ValidationContext; +import dev.wogu.api.ValidatorRunOutcome; +import dev.wogu.api.Violation; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** Integration-style tests for WG013, exercised through {@link TemporalWorkflowValidator}. */ +class CatchThrowableOrErrorRuleTest { + + @TempDir Path sourceRoot; + + private final TemporalWorkflowValidator validator = new TemporalWorkflowValidator(); + + private void writeJavaFile(String relativePath, String content) throws IOException { + Path file = sourceRoot.resolve(relativePath); + Files.createDirectories(file.getParent()); + Files.writeString(file, content); + } + + private void writeWorkflowInterface() throws IOException { + writeJavaFile( + "com/example/PaymentWorkflow.java", + """ + package com.example; + + import io.temporal.workflow.WorkflowInterface; + import io.temporal.workflow.WorkflowMethod; + + @WorkflowInterface + public interface PaymentWorkflow { + @WorkflowMethod + void process(); + } + """); + } + + private ValidationContext context() { + return new TestValidationContext("test-project", sourceRoot, List.of(sourceRoot)); + } + + private RuleResult wg013Result() { + ValidatorRunOutcome outcome = validator.validate(context()); + return outcome.ruleResults().stream() + .filter(result -> result.rule().id().equals("WG013")) + .findFirst() + .orElseThrow(); + } + + @Test + void flagsCatchingThrowableDirectlyInsideAWorkflowImplementation() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void process() { + try { + riskyStep(); + } catch (Throwable t) { + // swallowed + } + } + + private void riskyStep() {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.passed()).isFalse(); + assertThat(wg013.violations()).hasSize(1); + Violation violation = wg013.violations().get(0); + assertThat(violation.rule().id()).isEqualTo("WG013"); + assertThat(violation.className()).isEqualTo("PaymentWorkflowImpl"); + assertThat(violation.message()).contains("control-flow signals"); + assertThat(violation.suggestedFix()).contains("specific checked or application exceptions"); + assertThat(violation.callPath()) + .extracting(CallPathFrame::displayName) + .containsExactly("PaymentWorkflowImpl.process()", "catch (Throwable)"); + } + + @Test + void flagsCatchingErrorDirectlyInsideAWorkflowImplementation() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void process() { + try { + riskyStep(); + } catch (Error e) { + // swallowed + } + } + + private void riskyStep() {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.violations()).hasSize(1); + assertThat(wg013.violations().get(0).callPath()) + .extracting(CallPathFrame::displayName) + .containsExactly("PaymentWorkflowImpl.process()", "catch (Error)"); + } + + @Test + void flagsCatchingThrowableInsideAHelperMethodCalledFromTheWorkflow() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + private final PaymentService paymentService = new PaymentService(); + + @Override + public void process() { + paymentService.chargeCardSafely(); + } + } + """); + writeJavaFile( + "com/example/PaymentService.java", + """ + package com.example; + + public class PaymentService { + void chargeCardSafely() { + try { + chargeCard(); + } catch (Throwable t) { + // swallowed + } + } + + private void chargeCard() {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.violations()).hasSize(1); + Violation violation = wg013.violations().get(0); + assertThat(violation.className()).isEqualTo("PaymentService"); + assertThat(violation.callPath()) + .extracting(CallPathFrame::displayName) + .containsExactly("PaymentWorkflowImpl.process()", "PaymentService.chargeCardSafely()", "catch (Throwable)"); + } + + @Test + void flagsTheThrowableComponentOfAMultiCatch() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + import java.io.IOException; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void process() { + try { + riskyStep(); + } catch (IOException | Throwable t) { + // swallowed + } + } + + private void riskyStep() throws IOException {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.violations()).hasSize(1); + assertThat(wg013.violations().get(0).callPath()) + .extracting(CallPathFrame::displayName) + .containsExactly("PaymentWorkflowImpl.process()", "catch (Throwable)"); + } + + @Test + void stillFlagsCatchingThrowableInsideWorkflowSideEffect() throws IOException { + // Unlike WG001's suppressedContexts: SIDE_EFFECT, WG013 is not suppressed inside a + // side effect: catching Throwable/Error can swallow Temporal's internal control-flow + // signals on the workflow thread regardless of which API triggered that code path. + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + import io.temporal.workflow.Workflow; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void process() { + Workflow.sideEffect(Boolean.class, () -> { + try { + riskyStep(); + } catch (Throwable t) { + // swallowed + } + return true; + }); + } + + private void riskyStep() {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.violations()).hasSize(1); + } + + @Test + void doesNotFlagCatchingASpecificCheckedException() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + import java.io.IOException; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + @Override + public void process() { + try { + riskyStep(); + } catch (IOException e) { + // handled + } + } + + private void riskyStep() throws IOException {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.passed()).isTrue(); + assertThat(wg013.violations()).isEmpty(); + } + + @Test + void doesNotFlagCatchingThrowableInsideAnActivityImplementation() throws IOException { + writeWorkflowInterface(); + writeJavaFile( + "com/example/PaymentWorkflowImpl.java", + """ + package com.example; + + public class PaymentWorkflowImpl implements PaymentWorkflow { + private PaymentActivity activity; + + @Override + public void process() { + activity.chargeCard(); + } + } + """); + writeJavaFile( + "com/example/PaymentActivity.java", + """ + package com.example; + + import io.temporal.activity.ActivityInterface; + import io.temporal.activity.ActivityMethod; + + @ActivityInterface + public interface PaymentActivity { + @ActivityMethod + void chargeCard(); + } + """); + writeJavaFile( + "com/example/PaymentActivityImpl.java", + """ + package com.example; + + public class PaymentActivityImpl implements PaymentActivity { + @Override + public void chargeCard() { + try { + reallyChargeCard(); + } catch (Throwable t) { + // swallowed, but this is Activity code, not subject to WG013 + } + } + + private void reallyChargeCard() {} + } + """); + + RuleResult wg013 = wg013Result(); + + assertThat(wg013.passed()).isTrue(); + assertThat(wg013.violations()).isEmpty(); + } +} diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/MutableSideEffectEqualityRuleTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/MutableSideEffectEqualityRuleTest.java index 51f6c75..5b56f11 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/MutableSideEffectEqualityRuleTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/MutableSideEffectEqualityRuleTest.java @@ -377,6 +377,7 @@ public class Price { List.of(), List.of("SIDE_EFFECT"), 1, + List.of(), List.of()); TemporalRule rule = new MutableSideEffectEqualityRule(definitionRequiringSideEffect); diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionLoaderTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionLoaderTest.java index 12ae728..7beaac0 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionLoaderTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionLoaderTest.java @@ -215,6 +215,53 @@ void defaultsValueTypeArgumentIndexToNullWhenAbsent() { assertThat(definition.valueTypeArgumentIndex()).isNull(); } + @Test + void parsesCatchTypesWhenPresent() { + RuleDefinition definition = + loader.parse( + """ + id: WG013 + type: forbidden-catch-type + title: Example Rule + category: Determinism + severity: ERROR + engine: Temporal Java SDK + since: 1.0.0 + documentation: docs/rules/WG013.md + description: Example description. + replacement: Example replacement. + catchTypes: + - java.lang.Throwable + - java.lang.Error + """, + "test.yaml"); + + assertThat(definition.catchTypes()).containsExactly("java.lang.Throwable", "java.lang.Error"); + } + + @Test + void defaultsCatchTypesToEmptyWhenAbsent() { + RuleDefinition definition = + loader.parse( + """ + id: WG900 + type: forbidden-method + title: Example Rule + category: Determinism + severity: ERROR + engine: Temporal Java SDK + since: 0.2.0 + documentation: docs/rules/WG900.md + description: Example description. + replacement: Example replacement. + methods: + - java.util.UUID.randomUUID + """, + "test.yaml"); + + assertThat(definition.catchTypes()).isEmpty(); + } + @Test void rejectsADefinitionMissingARequiredField() { String missingSeverity = @@ -253,13 +300,17 @@ void loadAllFindsTheRealRuleDefinitionsPackagedInThisModule() { .extracting(RuleDefinition::id) .containsExactlyInAnyOrder( "WG001", "WG002", "WG003", "WG004", "WG005", "WG006", "WG007", "WG008", "WG009", "WG010", "WG011", - "WG012"); + "WG012", "WG013"); assertThat(definitions) - .filteredOn(definition -> !definition.id().equals("WG012")) + .filteredOn(definition -> !definition.id().equals("WG012") && !definition.id().equals("WG013")) .allSatisfy(definition -> assertThat(definition.type()).isEqualTo("forbidden-method")); assertThat(definitions) .filteredOn(definition -> definition.id().equals("WG012")) .singleElement() .satisfies(definition -> assertThat(definition.type()).isEqualTo("mutable-side-effect-equality")); + assertThat(definitions) + .filteredOn(definition -> definition.id().equals("WG013")) + .singleElement() + .satisfies(definition -> assertThat(definition.type()).isEqualTo("forbidden-catch-type")); } } diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionTest.java index b4e045f..90ac874 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionTest.java @@ -27,6 +27,7 @@ private static RuleDefinition definitionWith(String id, String category, String List.of(), List.of(), null, + List.of(), List.of()); } diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleRegistryTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleRegistryTest.java index 4d0ae0e..22b83b6 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/RuleRegistryTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/RuleRegistryTest.java @@ -10,14 +10,14 @@ class RuleRegistryTest { @Test - void loadsExactlyWG001ThroughWG012FromTheClasspath() { + void loadsExactlyWG001ThroughWG013FromTheClasspath() { List rules = RuleRegistry.loadDeclarativeRules(getClass().getClassLoader()); assertThat(rules) .extracting(rule -> rule.metadata().id()) .containsExactlyInAnyOrder( "WG001", "WG002", "WG003", "WG004", "WG005", "WG006", "WG007", "WG008", "WG009", "WG010", "WG011", - "WG012"); + "WG012", "WG013"); } @Test @@ -25,12 +25,16 @@ void everyLoadedRuleIsTheImplementationMatchingItsDeclaredType() { List rules = RuleRegistry.loadDeclarativeRules(getClass().getClassLoader()); assertThat(rules) - .filteredOn(rule -> !rule.metadata().id().equals("WG012")) + .filteredOn(rule -> !rule.metadata().id().equals("WG012") && !rule.metadata().id().equals("WG013")) .allSatisfy(rule -> assertThat(rule).isInstanceOf(ForbiddenMethodRule.class)); assertThat(rules) .filteredOn(rule -> rule.metadata().id().equals("WG012")) .singleElement() .isInstanceOf(MutableSideEffectEqualityRule.class); + assertThat(rules) + .filteredOn(rule -> rule.metadata().id().equals("WG013")) + .singleElement() + .isInstanceOf(ForbiddenCatchTypeRule.class); } @Test @@ -52,6 +56,7 @@ void rejectsADefinitionDeclaringAnUnknownType() { List.of(), List.of(), null, + List.of(), List.of()); assertThatThrownBy(() -> RuleRegistry.create(unknownType)) @@ -79,6 +84,7 @@ void createsAForbiddenMethodRuleWithMetadataMatchingTheDefinition() { List.of(), List.of(), null, + List.of(), List.of()); TemporalRule rule = RuleRegistry.create(definition); @@ -108,6 +114,7 @@ void createsAMutableSideEffectEqualityRuleWithMetadataMatchingTheDefinition() { List.of(), List.of(), 1, + List.of(), List.of()); TemporalRule rule = RuleRegistry.create(definition); @@ -138,6 +145,7 @@ void rejectsAMutableSideEffectEqualityDefinitionWithoutExactlyOneMethod() { List.of(), List.of(), 1, + List.of(), List.of()); assertThatThrownBy(() -> RuleRegistry.create(zeroMethods)) @@ -165,6 +173,7 @@ void rejectsAMutableSideEffectEqualityDefinitionMissingValueTypeArgumentIndex() List.of(), List.of(), null, + List.of(), List.of()); assertThatThrownBy(() -> RuleRegistry.create(noArgumentIndex)) @@ -172,4 +181,63 @@ void rejectsAMutableSideEffectEqualityDefinitionMissingValueTypeArgumentIndex() .hasMessageContaining("WG051") .hasMessageContaining("valueTypeArgumentIndex"); } + + @Test + void createsAForbiddenCatchTypeRuleWithMetadataMatchingTheDefinition() { + RuleDefinition definition = + new RuleDefinition( + "WG052", + "forbidden-catch-type", + "Example Rule", + "Example description", + "Determinism", + "ERROR", + "Temporal Java SDK", + "0.2.0", + "docs/rules/WG052.md", + "Example replacement", + List.of(), + List.of(), + List.of(), + List.of(), + null, + List.of("java.lang.Throwable"), + List.of()); + + TemporalRule rule = RuleRegistry.create(definition); + Rule metadata = rule.metadata(); + + assertThat(rule).isInstanceOf(ForbiddenCatchTypeRule.class); + assertThat(metadata.id()).isEqualTo("WG052"); + assertThat(metadata.title()).isEqualTo("Example Rule"); + assertThat(metadata.documentationReference()).isEqualTo("docs/rules/WG052.md"); + } + + @Test + void rejectsAForbiddenCatchTypeDefinitionWithNoCatchTypes() { + RuleDefinition noCatchTypes = + new RuleDefinition( + "WG052", + "forbidden-catch-type", + "Example Rule", + "Example description", + "Determinism", + "ERROR", + "Temporal Java SDK", + "0.2.0", + "docs/rules/WG052.md", + "Example replacement", + List.of(), + List.of(), + List.of(), + List.of(), + null, + List.of(), + List.of()); + + assertThatThrownBy(() -> RuleRegistry.create(noCatchTypes)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("WG052") + .hasMessageContaining("catchTypes"); + } } diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/TemporalWorkflowValidatorTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/TemporalWorkflowValidatorTest.java index 9a3faa3..1e68b46 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/TemporalWorkflowValidatorTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/TemporalWorkflowValidatorTest.java @@ -33,12 +33,12 @@ private ValidationContext context() { } @Test - void declaresWG001ThroughWG012AsItsRules() { + void declaresWG001ThroughWG013AsItsRules() { assertThat(validator.rules()) .extracting(Rule::id) .containsExactlyInAnyOrder( "WG001", "WG002", "WG003", "WG004", "WG005", "WG006", "WG007", "WG008", "WG009", "WG010", "WG011", - "WG012"); + "WG012", "WG013"); } @Test From 9a5531f401b3f6c7a8389eb426f25af947f97fb2 Mon Sep 17 00:00:00 2001 From: Vikas Pandey Date: Mon, 13 Jul 2026 20:40:48 +0530 Subject: [PATCH 2/2] WG013: fix invalid multi-catch in test fixture catch (IOException | Throwable t) would never compile: JLS 14.20 forbids a multi-catch alternative that is a subclass of another alternative in the same clause, and IOException is a subclass of Throwable. JavaParser's grammar-level parser doesn't enforce that semantic rule, so the test still passed mechanically despite exercising code no real project could contain. Swapped to IOException | Error (siblings under Throwable, a valid disjoint multi-catch) and updated the expected call-path frame and test name to match. Co-Authored-By: Claude Sonnet 5 --- .../wogu/temporal/CatchThrowableOrErrorRuleTest.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java b/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java index ad2b1e9..28fc33a 100644 --- a/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java +++ b/wogu-temporal/src/test/java/dev/wogu/temporal/CatchThrowableOrErrorRuleTest.java @@ -168,7 +168,12 @@ private void chargeCard() {} } @Test - void flagsTheThrowableComponentOfAMultiCatch() throws IOException { + void flagsTheErrorComponentOfAMultiCatch() throws IOException { + // IOException | Throwable would not compile: JLS 14.20 forbids a multi-catch + // alternative that is a subclass of another alternative in the same clause, and + // IOException is a subclass of Throwable. Error is a sibling of Exception under + // Throwable, so IOException | Error is a valid, disjoint multi-catch that still + // exercises the same component-by-component matching. writeWorkflowInterface(); writeJavaFile( "com/example/PaymentWorkflowImpl.java", @@ -182,7 +187,7 @@ public class PaymentWorkflowImpl implements PaymentWorkflow { public void process() { try { riskyStep(); - } catch (IOException | Throwable t) { + } catch (IOException | Error t) { // swallowed } } @@ -196,7 +201,7 @@ private void riskyStep() throws IOException {} assertThat(wg013.violations()).hasSize(1); assertThat(wg013.violations().get(0).callPath()) .extracting(CallPathFrame::displayName) - .containsExactly("PaymentWorkflowImpl.process()", "catch (Throwable)"); + .containsExactly("PaymentWorkflowImpl.process()", "catch (Error)"); } @Test