From 16d033d64aaaf48fad97ed466f57390dd0e41a04 Mon Sep 17 00:00:00 2001 From: Zhoujie628 Date: Thu, 17 Sep 2026 16:03:08 +0800 Subject: [PATCH] refactor(model): defensively copy collection fields, expose unmodifiable views Model types returned raw references to their internal collections, so a caller mutating a getter result silently corrupted engine-owned snapshots and could leak across concurrent executions. Collection fields on Workflow, WorkflowStep, WorkflowSearchResult, ExecutionResult, SendMessageResult, TaskResult, BusinessFailure and the message records are now copied on write and exposed through unmodifiable views. Source- and binary-compatible: no signatures change, and no call site in the reactor mutates a returned collection. Callers that do mutate getter results now receive UnsupportedOperationException instead of corrupting engine state; noted in the changelog for integrators. SpotBugs EI exclusions narrow accordingly: the blanket EI acceptance is replaced by targeted rules for Lombok-generated builders (build() performs the copies) and five documented engine-internal acceptances, so future model code with raw exposure fails the gate. Copies are null-tolerant where entries may be null, preserving graph-validation semantics covered by WorkflowExecutorTest. Signed-off-by: Zhoujie628 --- CHANGELOG.md | 5 ++ config/spotbugs/spotbugs-exclude.xml | 44 +++++++++++++-- .../engine/model/BusinessFailure.java | 4 +- .../engine/model/ExecutionResult.java | 40 ++++++++++++-- .../workflow/engine/model/MessageContent.java | 18 +++++++ .../engine/model/ReceivedMessage.java | 12 +++++ .../engine/model/SendMessageResult.java | 17 +++++- .../engine/model/TaskExecutionResult.java | 12 +++++ .../workflow/engine/model/TaskResult.java | 10 ++++ .../workflow/engine/model/Workflow.java | 21 +++++++- .../engine/model/WorkflowSearchResult.java | 37 ++++++++++++- .../workflow/engine/model/WorkflowStep.java | 54 ++++++++++++++++++- 12 files changed, 257 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32a0b6b1..2e992c27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ interval (`a2at.server.heartbeat-interval-seconds`, default 15 seconds, `0` disables). Keeps intermediaries from idle-timing-out long-running SSE connections during agent execution; the heartbeat integrates with the emitter lifecycle and stops on stream completion, error, timeout, or client disconnect. +- Make collection fields on model types defensively copied and expose them through unmodifiable + views (`Workflow`, `WorkflowStep`, `WorkflowSearchResult`, `ExecutionResult`, `SendMessageResult`, + `TaskResult`, `BusinessFailure`, and the message records). Source- and binary-compatible; callers + that mutate the collections returned by these getters now receive `UnsupportedOperationException` + instead of silently corrupting engine-owned snapshots. ## [0.0.10] — 2026-09-16 diff --git a/config/spotbugs/spotbugs-exclude.xml b/config/spotbugs/spotbugs-exclude.xml index e49f56af..b2333041 100644 --- a/config/spotbugs/spotbugs-exclude.xml +++ b/config/spotbugs/spotbugs-exclude.xml @@ -5,16 +5,43 @@ for this codebase's patterns. Clean up over time; new findings still block verify. --> - + + - + + + + + + + + + + + + + + + + + + + + + @@ -69,13 +96,19 @@ - NegotiationStrategy intentionally shadows the control interface name (sample mirror) - Boolean-returning booleanParam helper may return null to signal "unset" - handleLogin catches Exception broadly as the demo login boundary - - doPrivileged warning: SecurityManager is not used by this project --> + - doPrivileged warning: SecurityManager is not used by this project + - OmcAgentLauncher.servers(): demo server registry, iterated by the demo only + - SpringWorkbenchExecutor/SpringWorkbenchExtensionLifecycle: storing the Spring + properties bean reference is the standard Spring configuration pattern --> + + + @@ -86,6 +119,9 @@ + + + diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/BusinessFailure.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/BusinessFailure.java index f7f94aeb..4b829ff3 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/BusinessFailure.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/BusinessFailure.java @@ -39,8 +39,8 @@ public String code() { return code; } - /** Safe business facts selected by the host. */ + /** Safe business facts selected by the host; unmodifiable. */ public Map details() { - return details; + return java.util.Collections.unmodifiableMap(details); } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ExecutionResult.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ExecutionResult.java index 645bb43a..8111a51b 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ExecutionResult.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ExecutionResult.java @@ -4,7 +4,7 @@ * * SPDX-License-Identifier: Apache-2.0 * - * Licensed under the Apache License, Version 2.0 (the License); you may + * Licensed under the Apache License, Version 2.0 (the "License"); you may * not use this file except in compliance with the License. You may obtain * a copy of the License at * @@ -19,9 +19,11 @@ package dev.openan.workflow.engine.model; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; @@ -29,15 +31,17 @@ /** * Outcome of one workflow run: overall success, per-step execution history, aggregated step * outputs, and a terminal error description when the run failed. + * + *

Collection fields are copied on write and exposed unmodifiable, so callers cannot mutate the + * result after the engine has produced it. */ @Data @NoArgsConstructor -@AllArgsConstructor @Builder public class ExecutionResult { private boolean success; - /** Per-step records in execution order. */ + /** Per-step records in execution order. Stored defensively; exposed unmodifiable. */ private List> history; /** Step name to business outputs; nested structure preserved. */ @@ -45,4 +49,32 @@ public class ExecutionResult { /** Terminal failure description; raw error text, not redacted. Null on success. */ private String error; + + /** All-args constructor with defensive copies; Lombok's builder routes through it. */ + public ExecutionResult( + boolean success, + List> history, + Map> stepOutputs, + String error) { + this.success = success; + this.history = history == null ? null : new ArrayList<>(history); + this.stepOutputs = stepOutputs == null ? null : new HashMap<>(stepOutputs); + this.error = error; + } + + public List> getHistory() { + return history == null ? null : Collections.unmodifiableList(history); + } + + public void setHistory(List> history) { + this.history = history == null ? null : new ArrayList<>(history); + } + + public Map> getStepOutputs() { + return stepOutputs == null ? null : Collections.unmodifiableMap(stepOutputs); + } + + public void setStepOutputs(Map> stepOutputs) { + this.stepOutputs = stepOutputs == null ? null : new HashMap<>(stepOutputs); + } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/MessageContent.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/MessageContent.java index 3edd041b..0cb714d6 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/MessageContent.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/MessageContent.java @@ -56,6 +56,24 @@ public record MessageContent( extensions = Collections.unmodifiableSet(active); } + /** Unmodifiable view; the underlying snapshot is immutable by construction. */ + @Override + public List> parts() { + return Collections.unmodifiableList(parts); + } + + /** Unmodifiable view; the underlying snapshot is immutable by construction. */ + @Override + public Map metadata() { + return Collections.unmodifiableMap(metadata); + } + + /** Unmodifiable view; the underlying set is immutable by construction. */ + @Override + public Set extensions() { + return Collections.unmodifiableSet(extensions); + } + /** Plain A2A text, without implicit extension activation or generation. */ public static MessageContent text(String text) { return parts(List.of(new TextPart(Objects.requireNonNull(text, "text")))); diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ReceivedMessage.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ReceivedMessage.java index e6f489ff..1d025db8 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ReceivedMessage.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/ReceivedMessage.java @@ -49,6 +49,18 @@ public record ReceivedMessage( .toList(); } + /** Unmodifiable view; artifacts are defensive snapshots. */ + @Override + public Map taskMetadata() { + return java.util.Collections.unmodifiableMap(taskMetadata); + } + + /** Unmodifiable view; artifacts are defensive snapshots. */ + @Override + public List artifacts() { + return java.util.Collections.unmodifiableList(artifacts); + } + private static void project(List> parts, List values) { for (Part part : parts) { if (part instanceof TextPart text) { diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/SendMessageResult.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/SendMessageResult.java index e789b647..fde6994c 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/SendMessageResult.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/SendMessageResult.java @@ -78,8 +78,23 @@ public List getOutputs() { .toList(); } + /** Unmodifiable view; stored via a defensive JSON snapshot. */ + public Map getMetadata() { + return metadata == null ? null : java.util.Collections.unmodifiableMap(metadata); + } + + /** Stores a defensive JSON snapshot of the supplied metadata. */ + public void setMetadata(Map metadata) { + this.metadata = metadata == null ? Map.of() : BusinessValues.map(metadata); + } + + /** Unmodifiable view of the structured response projection. */ + public List getReceivedMessages() { + return java.util.Collections.unmodifiableList(receivedMessages); + } + /** Snapshots the list supplied by transport adapters. */ public void setReceivedMessages(List messages) { - this.receivedMessages = List.copyOf(messages); + this.receivedMessages = messages == null ? List.of() : List.copyOf(messages); } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskExecutionResult.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskExecutionResult.java index 5daa5bfb..75754e58 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskExecutionResult.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskExecutionResult.java @@ -50,4 +50,16 @@ public record TaskExecutionResult( : receivedMessages.stream().flatMap(message -> message.outputs().stream()).toList(); errorDetails = errorDetails == null ? java.util.Map.of() : BusinessValues.map(errorDetails); } + + /** Unmodifiable view; outputs are defensive snapshots. */ + @Override + public List outputs() { + return java.util.Collections.unmodifiableList(outputs); + } + + /** Unmodifiable view; error details are defensive snapshots. */ + @Override + public java.util.Map errorDetails() { + return java.util.Collections.unmodifiableMap(errorDetails); + } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskResult.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskResult.java index dfde9bc5..0910681f 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskResult.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/TaskResult.java @@ -79,4 +79,14 @@ public static TaskResult success(List outputs) { public static TaskResult failure(String code, String message) { return builder().success(false).errorCode(code).error(message).build(); } + + /** Unmodifiable view; outputs are defensive snapshots. */ + public List getOutputs() { + return java.util.Collections.unmodifiableList(outputs); + } + + /** Unmodifiable view; error details are defensive snapshots. */ + public java.util.Map getErrorDetails() { + return java.util.Collections.unmodifiableMap(errorDetails); + } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/Workflow.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/Workflow.java index 2b6eab91..fed41b11 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/Workflow.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/Workflow.java @@ -20,9 +20,9 @@ package dev.openan.workflow.engine.model; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; @@ -35,7 +35,6 @@ */ @Data @NoArgsConstructor -@AllArgsConstructor @Builder public class Workflow { @Builder.Default private String id = ""; @@ -43,6 +42,24 @@ public class Workflow { @Builder.Default private String description = ""; @Builder.Default private List steps = List.of(); + /** All-args constructor with defensive copies; Lombok's builder routes through it. */ + public Workflow(String id, String name, String description, List steps) { + this.id = id == null ? "" : id; + this.name = name == null ? "" : name; + this.description = description == null ? "" : description; + this.steps = steps == null ? List.of() : new ArrayList<>(steps); + } + + /** Unmodifiable view; steps are snapshots of the loaded definition. */ + public List getSteps() { + return steps == null ? null : Collections.unmodifiableList(steps); + } + + /** Stores a defensive copy of the supplied step list. */ + public void setSteps(List steps) { + this.steps = steps == null ? List.of() : new ArrayList<>(steps); + } + /** * Builds a workflow from a PSOP-style map (as returned by the orchestration center). * diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowSearchResult.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowSearchResult.java index 48cc71f7..4cf1d789 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowSearchResult.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowSearchResult.java @@ -20,8 +20,8 @@ package dev.openan.workflow.engine.model; import dev.openan.workflow.engine.registry.LoadPsop; +import java.util.Collections; import java.util.List; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; @@ -35,7 +35,6 @@ */ @Data @NoArgsConstructor -@AllArgsConstructor @Builder public class WorkflowSearchResult { private String workflowId; @@ -48,4 +47,38 @@ public class WorkflowSearchResult { private String userIntent; private String relatedPreflow; private String tasksSummary; + + /** All-args constructor with defensive copies; Lombok's builder routes through it. */ + public WorkflowSearchResult( + String workflowId, + String workflowType, + String name, + String description, + List tags, + String createdAt, + double score, + String userIntent, + String relatedPreflow, + String tasksSummary) { + this.workflowId = workflowId; + this.workflowType = workflowType; + this.name = name; + this.description = description; + this.tags = tags == null ? List.of() : List.copyOf(tags); + this.createdAt = createdAt; + this.score = score; + this.userIntent = userIntent; + this.relatedPreflow = relatedPreflow; + this.tasksSummary = tasksSummary; + } + + /** Unmodifiable view; tags are snapshots. */ + public List getTags() { + return tags == null ? null : Collections.unmodifiableList(tags); + } + + /** Stores a defensive copy of the supplied tag list. */ + public void setTags(List tags) { + this.tags = tags == null ? List.of() : List.copyOf(tags); + } } diff --git a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowStep.java b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowStep.java index 6b6d8f09..bd7a60ac 100644 --- a/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowStep.java +++ b/workflow-engine/src/main/java/dev/openan/workflow/engine/model/WorkflowStep.java @@ -19,8 +19,9 @@ package dev.openan.workflow.engine.model; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; @@ -28,10 +29,11 @@ /** * One node of a {@link Workflow}: the subtasks to dispatch, the outgoing jumps, and the upstream * window its callbacks receive. + * + *

Collection fields are copied on write and exposed unmodifiable. */ @Data @NoArgsConstructor -@AllArgsConstructor @Builder public class WorkflowStep { /** @@ -65,4 +67,52 @@ public class WorkflowStep { * through {@code onSelfTask} with no agent network calls. */ @Builder.Default private StepType stepType = StepType.ALL_SUCCESS; + + /** All-args constructor with defensive copies; Lombok's builder routes through it. */ + public WorkflowStep( + String name, + List subtasks, + List next, + int layer, + List contextFrom, + StepType stepType) { + this.name = name; + // Null-tolerant copies: null edge/task entries are reported by graph validation, + // not silently rejected at construction. + this.subtasks = subtasks == null ? List.of() : new ArrayList<>(subtasks); + this.next = next == null ? List.of() : new ArrayList<>(next); + this.layer = layer; + this.contextFrom = contextFrom == null ? null : new ArrayList<>(contextFrom); + this.stepType = stepType; + } + + /** Unmodifiable view; subtasks are snapshots. */ + public List getSubtasks() { + return subtasks == null ? null : Collections.unmodifiableList(subtasks); + } + + /** Stores a null-tolerant defensive copy of the supplied subtask list. */ + public void setSubtasks(List subtasks) { + this.subtasks = subtasks == null ? List.of() : new ArrayList<>(subtasks); + } + + /** Unmodifiable view; edges are snapshots. */ + public List getNext() { + return next == null ? null : Collections.unmodifiableList(next); + } + + /** Stores a null-tolerant defensive copy; invalid entries are caught by graph validation. */ + public void setNext(List next) { + this.next = next == null ? List.of() : new ArrayList<>(next); + } + + /** Unmodifiable view; {@code null} selects direct predecessors. */ + public List getContextFrom() { + return contextFrom == null ? null : Collections.unmodifiableList(contextFrom); + } + + /** Stores a defensive copy; {@code null} selects direct predecessors. */ + public void setContextFrom(List contextFrom) { + this.contextFrom = contextFrom == null ? null : new ArrayList<>(contextFrom); + } }