Add eval for replacing stream forEach mutation with result-producing collectors

[martinfrancois]

Problem

Add a Java Streams skill eval for refactoring stream pipelines that look stream-based but still mutate external collections or maps with forEach(...).

The lesson is not that every forEach(...) is wrong. The lesson is that when the operation is to build a returned list or map, the stream should normally produce that result directly with Stream.concat(...), toList(), or a collector instead of mutating a collection or map that lives outside the pipeline.

This is eval-worthy because the original code is correct, but the stream usage is misleading: the stream does transformation work and then hands accumulation back to external mutable state.

Code before the prompt was executed

The first method built a list of configured and environment-provided paths by mutating an external list from two stream terminal operations, then streaming the list again to remove duplicates.

private List<Path> additionalWritableRoots(Path workflowDirectory, Map<String, Object> codex) {
    List<Path> roots = new ArrayList<>();
    list(codex, "additional_writable_roots", List.of()).stream()
            .map(value -> path(workflowDirectory, value))
            .forEach(roots::add);
    environmentValue("SYMPHONY_CODEX_ADDITIONAL_WRITABLE_ROOTS").stream()
            .flatMap(value -> Arrays.stream(value.split(Pattern.quote(File.pathSeparator))))
            .map(String::trim)
            .filter(value -> !value.isBlank())
            .map(value -> path(workflowDirectory, value))
            .forEach(roots::add);
    return roots.stream().distinct().toList();
}

The second method counted normalized states by mutating an external map from a stream terminal operation.

private Map<String, Integer> runningCountsByState() {
    Map<String, Integer> counts = new HashMap<>();
    running.values().stream()
            .map(entry -> StateNames.normalize(entry.card.state()))
            .forEach(state -> counts.merge(state, 1, Integer::sum));
    return counts;
}

Prompt that caused the implementation

A maintainer asked to continue the existing cleanup plan after merging the skill-installation PR. The active work item was to sweep the Java sources and apply the Java Streams skill where it made code meaningfully better.

Relevant instruction:

continue with our plan

The plan item was to use the installed Java Optional and Java Streams skills to clean up production code where the skill guidance identifies a contained, behavior-preserving improvement.

Later prompt that exposed the issue

There was no later maintainer correction for these two methods. The cleanup was found by applying the Streams skill during the planned source sweep.

Prompt-produced code before maintainer correction

There was no separate rejected intermediate implementation. The code in the first section is the pre-refactor code being corrected.

Why the old code is bad

The old code is not bad because it produces the wrong result. It is bad because the stream pipeline does not own the result it is building.

1. forEach(roots::add) and forEach(state -> counts.merge(...)) mutate external structures.
2. The stream is doing transformation work, but accumulation happens outside the pipeline.
3. The list method allocates a mutable intermediate list only so it can stream it again for distinct().
4. The map method hides the counting operation behind a side-effecting merge lambda.
5. The code makes the reader audit external mutation when a result-producing terminal operation can state the intent directly.

Maintainer-preferred code

The list-building method should let the stream produce the final list. The two ordered sources are named and then concatenated before duplicate removal.

private List<Path> additionalWritableRoots(Path workflowDirectory, Map<String, Object> codex) {
    Stream<Path> configuredRoots = list(codex, "additional_writable_roots", List.of()).stream()
            .map(value -> path(workflowDirectory, value));
    Stream<Path> environmentRoots = environmentValue("SYMPHONY_CODEX_ADDITIONAL_WRITABLE_ROOTS").stream()
            .flatMap(value -> Arrays.stream(value.split(Pattern.quote(File.pathSeparator))))
            .map(String::trim)
            .filter(value -> !value.isBlank())
            .map(value -> path(workflowDirectory, value));
    return Stream.concat(configuredRoots, environmentRoots).distinct().toList();
}

The counting method should use a collector with explicit duplicate-key merge behavior and an explicit mutable map type.

private Map<String, Integer> runningCountsByState() {
    return running.values().stream()
            .map(entry -> StateNames.normalize(entry.card.state()))
            .collect(Collectors.toMap(state -> state, state -> 1, Integer::sum, HashMap::new));
}

Why the replacement is better

The replacement makes each stream pipeline produce the requested value directly.

For the list case:

• configuredRoots and environmentRoots name the two ordered sources.
• Stream.concat(...) preserves the original precedence order: configured roots first, environment roots second.
• distinct() still removes duplicates using encounter order.
• toList() returns the same unmodifiable result shape as the old final roots.stream().distinct().toList().
• The unnecessary mutable ArrayList disappears.

For the map case:

• Collectors.toMap(...) states that the result is a map from normalized state to count.
• The merge function Integer::sum states how duplicate states are counted.
• HashMap::new preserves the previous concrete map behavior where callers received a mutable HashMap-style result.
• The external side-effecting counts.merge(...) lambda disappears.

Desired eval behavior

• Reward identifying stream().forEach(...) external collection or map mutation as a refactor target when the goal is a returned value.
• Reward using Stream.concat(...) when two ordered sources should be combined before later stream operations.
• Reward preserving encounter order and duplicate handling when replacing list mutation with a stream pipeline.
• Reward Collectors.toMap(...) with a merge function when duplicate keys are expected.
• Reward preserving result mutability or concrete map behavior when surrounding code may rely on it.
• Reward explaining that the old code is functionally correct but less clear and less idiomatic.

Anti-patterns the eval should reject

• Replacing the mutable list with two separate lists and concatenating them imperatively.
• Keeping forEach(roots::add) or forEach(state -> counts.merge(...)) as the final answer.
• Using Collectors.toMap(...) without a merge function for data that can contain duplicate keys.
• Changing the list result from ordered first occurrence to an unordered set.
• Changing the map result to an immutable map if the original method returned a mutable map and mutability was not audited.
• Adding parallelStream() or concurrent collectors without measured CPU-heavy work and a concurrency requirement.
• Claiming the direct collector version is guaranteed faster instead of describing it as a readability and side-effect-boundary improvement.

Suggested eval name

collector-owned-result-instead-of-foreach-mutation

[martinfrancois]

Additional same-pattern occurrences from a later cleanup pass. These are useful eval additions because they show two related outcomes the skill should choose between:

• when a stream builds a returned value, use toList() or a collector;
• when the real purpose is mutation of an external API object, prefer a plain loop over a stream-shaped side effect.

Prompt that caused the cleanup

A maintainer asked for repeated Optional/Stream cleanup passes until the actionable pass was clean:

ARE YOU SURE you covered all optionals/streams things there are to cleanup with the skills? please do more passes until a pass comes back clean

Same-pattern code before cleanup: collector-owned result

SequencedSet<Path> unconnectedWorkflowPaths = new LinkedHashSet<>();
if (separateUnconnectedWorkflows) {
    reportWorkflowPaths(selectedManifest, selection.workflow(), paths.configDir(), true).stream()
            .filter(workflow -> selectedWorkflowPaths.stream()
                    .noneMatch(connected -> PathsEqual.samePath(connected, workflow)))
            .forEach(unconnectedWorkflowPaths::add);
}

Final code for that occurrence

SequencedSet<Path> unconnectedWorkflowPaths = separateUnconnectedWorkflows
        ? reportWorkflowPaths(selectedManifest, selection.workflow(), paths.configDir(), true).stream()
                .filter(workflow -> selectedWorkflowPaths.stream()
                        .noneMatch(connected -> PathsEqual.samePath(connected, workflow)))
                .collect(Collectors.toCollection(LinkedHashSet::new))
        : new LinkedHashSet<>();

Same-pattern code before cleanup: I/O loop plus local stream result

for (String listId : archivedListIds) {
    List<Map<String, Object>> listCards = getList(
            config,
            "lists/" + encodeSegment(listId) + "/cards",
            Map.of("fields", CARD_FIELDS, "filter", "all"));
    listCards.stream()
            .map(card -> normalize(card, context, config))
            .flatMap(Optional::stream)
            .filter(card -> isTerminal(card, config))
            .forEach(normalized::add);
}

Final code for that occurrence

for (String listId : archivedListIds) {
    List<Map<String, Object>> listCards = getList(
            config,
            "lists/" + encodeSegment(listId) + "/cards",
            Map.of("fields", CARD_FIELDS, "filter", "all"));
    List<Card> archivedTerminalCards = listCards.stream()
            .map(card -> normalize(card, context, config))
            .flatMap(Optional::stream)
            .filter(card -> isTerminal(card, config))
            .toList();
    normalized.addAll(archivedTerminalCards);
}

Same-pattern code before cleanup: mutation is the real operation

private static void appendWritableRoots(ObjectNode policy, List<Path> roots) {
    ArrayNode writableRoots = policy.withArray(WRITABLE_ROOTS);
    roots.stream()
            .map(Path::toString)
            .filter(root -> !containsText(writableRoots, root))
            .forEach(writableRoots::add);
}

Final code for that occurrence

private static void appendWritableRoots(ObjectNode policy, List<Path> roots) {
    ArrayNode writableRoots = policy.withArray(WRITABLE_ROOTS);
    for (Path root : roots) {
        String text = root.toString();
        if (!containsText(writableRoots, text)) {
            writableRoots.add(text);
        }
    }
}

Another same-boundary example used Jackson's JsonNode.forEach(models::add) to append into an ArrayNode; the cleanup used a plain for (JsonNode pageModel : pageModels) loop for the same reason.

Why these additions matter

The skill should not blindly replace every forEach with a collector. It should first identify the boundary:

• If the code is constructing a Java collection result, the stream should own the result.
• If the code is updating a mutable external API object such as a Jackson ArrayNode, a plain loop is clearer than stream-shaped mutation.
• If a loop performs I/O and then transforms each response, keep the I/O loop imperative and let the inner pure transformation produce a local value.

Desired eval behavior

Reward answers that classify the side-effect boundary before refactoring, then choose either a result-producing stream or a plain loop. Reject answers that keep forEach(...::add) as the final stream shape when the operation is building a returned collection.

[martinfrancois]

Additional same-pattern occurrences from another cleanup pass after the first comment.

Prompt that caused the cleanup

The maintainer asked for more passes until an actionable Optional/Stream pass came back clean:

ARE YOU SURE you covered all optioanls/streams thinfs ther3 are to cleanup with th3 skills? please do more passes until a pass comes back clean

Collector-owned result occurrence

Before:

filesByNumber.entrySet().stream()
        .filter(entry -> entry.getValue().size() > 1)
        .forEach(entry -> violations.add(
                "ADR number %s is used by multiple files: %s".formatted(entry.getKey(), entry.getValue())));

After:

List<String> duplicateNumberViolations = filesByNumber.entrySet().stream()
        .filter(entry -> entry.getValue().size() > 1)
        .map(entry ->
                "ADR number %s is used by multiple files: %s".formatted(entry.getKey(), entry.getValue()))
        .toList();
violations.addAll(duplicateNumberViolations);

Why: the stream now produces the list of violation messages instead of mutating the external violations list from the terminal operation.

Files.list Optional flattening occurrence

Before:

try (var files = Files.list(configDir)) {
    files.filter(Files::isRegularFile)
            .filter(file -> PathNames.fileName(file).endsWith(".md"))
            .filter(file -> ignoredBoard.workflowPath() == null
                    || !PathsEqual.samePath(file, ignoredBoard.workflowPath()))
            .forEach(file -> workflowConfig
                    .serverPort(file, WorkflowEnvironmentResolver.resolver(environment, options.envPath()))
                    .ifPresent(reserved::add));
}

After:

try (var files = Files.list(configDir)) {
    Set<Integer> workflowPorts = files.filter(Files::isRegularFile)
            .filter(file -> PathNames.fileName(file).endsWith(".md"))
            .filter(file -> ignoredBoard.workflowPath() == null
                    || !PathsEqual.samePath(file, ignoredBoard.workflowPath()))
            .map(file -> workflowConfig.serverPort(
                    file, WorkflowEnvironmentResolver.resolver(environment, options.envPath())))
            .flatMap(Optional::stream)
            .collect(Collectors.toSet());
    reserved.addAll(workflowPorts);
}

Why: the stream now owns discovery of the set of ports. It also uses flatMap(Optional::stream) instead of nesting ifPresent(...) inside forEach(...).

Mutation-boundary occurrence

Before:

manifest.boards().stream()
        .map(ConnectedBoard::workflowPath)
        .filter(Objects::nonNull)
        .forEach(workflow -> addUniqueWorkflow(workflowPaths, canonicalIdentities, workflow));
selectedWorkflow.ifPresent(workflow -> addUniqueWorkflow(workflowPaths, canonicalIdentities, workflow));
if (scanConfigDir) {
    workflowFilesIfReadable(configDir)
            .forEach(workflow -> addUniqueWorkflow(workflowPaths, canonicalIdentities, workflow));
}

After:

for (ConnectedBoard board : manifest.boards()) {
    if (board.workflowPath() != null) {
        addUniqueWorkflow(workflowPaths, canonicalIdentities, board.workflowPath());
    }
}
selectedWorkflow.ifPresent(workflow -> addUniqueWorkflow(workflowPaths, canonicalIdentities, workflow));
if (scanConfigDir) {
    for (Path workflow : workflowFilesIfReadable(configDir)) {
        addUniqueWorkflow(workflowPaths, canonicalIdentities, workflow);
    }
}

Why: this helper intentionally mutates two coordinated sets through addUniqueWorkflow(...). A plain loop exposes that stateful boundary better than a stream-shaped side effect.

Accumulator-boundary occurrence

Before:

plan.items().stream()
        .map(TrelloChecklistClassifier.PrerequisiteItem::reference)
        .forEach(reference -> references.add("checklist", reference.url(), reference));

After:

for (TrelloChecklistClassifier.PrerequisiteItem item : plan.items()) {
    TrelloCardReference reference = item.reference();
    references.add("checklist", reference.url(), reference);
}

Why: adding to a domain accumulator is the actual operation. A loop is clearer than using a stream only to drive references.add(...).

[martinfrancois]

Additional correction-chain occurrence for the same eval.

Prompt that caused the weaker intermediate code

During an Optional/Streams cleanup sweep, the maintainer asked whether fetchTerminalCards(EffectiveConfig) could be improved and specifically called out that changing forEach to addAll might be worse because it creates another heap object:

is there not a way to imrpove fetchTerminalCards(EffectiveConfig? it looks a bit snelly to me with multiple streams one after another, a for loop and im also notnsure the refactoring from forEach to the addAll is really better, because now we are creatijg another object that is on the heap unnecessarily ... it would be even better if it was possible to change it to not require forEach and not addAll if possible thohgh.

Code before the weaker intermediate

The loop performed one Trello read per archived list and used a stream only for the local response transformation. The terminal operation mutated the already-existing normalized result list:

for (String listId : archivedListIds) {
    List<Map<String, Object>> listCards = getList(
            config,
            "lists/" + encodeSegment(listId) + "/cards",
            Map.of("fields", CARD_FIELDS, "filter", "all"));
    listCards.stream()
            .map(card -> normalize(card, context, config))
            .flatMap(Optional::stream)
            .filter(card -> isTerminal(card, config))
            .forEach(normalized::add);
}

Prompt-produced code before maintainer correction

A later cleanup changed the stream terminal mutation into a local list plus addAll(...):

for (String listId : archivedListIds) {
    List<Map<String, Object>> listCards = getList(
            config,
            "lists/" + encodeSegment(listId) + "/cards",
            Map.of("fields", CARD_FIELDS, "filter", "all"));
    List<Card> archivedTerminalCards = listCards.stream()
            .map(card -> normalize(card, context, config))
            .flatMap(Optional::stream)
            .filter(card -> isTerminal(card, config))
            .toList();
    normalized.addAll(archivedTerminalCards);
}

Why the prompt-produced code is weak

This is behaviorally correct, but it is not a real improvement. The I/O loop is still imperative, the final target is still the existing mutable normalized list, and the refactor adds an intermediate list allocation on every archived list. The stream no longer mutates from the terminal operation, but the code still mutates normalized immediately afterward, so the side-effect boundary did not actually become clearer.

Behavior-equivalence analysis

The toList()+addAll(...) version preserves encounter order and result contents, but changes allocation behavior. In this boundary the extra allocation is not justified because there is no returned stream-owned result for the inner pipeline; the outer loop is doing Trello I/O and appending to an existing result list.

Maintainer-preferred code

The preferred cleanup keeps the I/O loop explicit and moves the append behavior into a named helper that avoids the intermediate list:

for (String listId : archivedListIds) {
    List<Map<String, Object>> listCards = getList(
            config,
            "lists/" + encodeSegment(listId) + "/cards",
            Map.of("fields", CARD_FIELDS, "filter", "all"));
    appendTerminalCards(normalized, listCards, context, config);
}

private void appendTerminalCards(
        List<Card> cards,
        List<Map<String, Object>> payloads,
        BoardContext context,
        EffectiveConfig config) {
    for (Map<String, Object> payload : payloads) {
        normalize(payload, context, config)
                .filter(card -> isTerminal(card, config))
                .ifPresent(cards::add);
    }
}

Desired eval behavior

Reward recognizing that forEach(...::add) is not always fixed by toList()+addAll(...). When the surrounding boundary is an imperative I/O loop and the destination list already exists, the eval should prefer either a direct named append helper or a fully result-owning stream only when it does not hide I/O and does not add needless allocation.

[martinfrancois]

Additional mutation-boundary occurrence from the same cleanup sweep.

Prompt that caused the cleanup

The maintainer asked for repeated Optional/Streams passes and for similar forEach(...add...) cases to be treated consistently:

i noticed tjere are other cases of forEqch ehign turned int9 addAll, treat all of them the same

Code before cleanup

The method was assembling Trello-visible Markdown lines. The stream terminal operations only existed to mutate the existing lines list:

if (!card.blockedBy().isEmpty()) {
    lines.add("");
    lines.add("Waiting for:");
    card.blockedBy().forEach(blocker -> lines.add("- " + waitingBlockerText(blocker)));
}
if (!card.prerequisiteProblems().isEmpty()) {
    lines.add("");
    lines.add("Checklist cleanup needed:");
    card.prerequisiteProblems()
            .forEach(problem -> lines.add("- " + problem.message()
                    + (blank(problem.checklist()) ? "" : " Checklist: " + problem.checklist() + ".")));
}

Maintainer-preferred code

A plain loop is clearer because the operation is ordered text assembly into an existing list, not production of a returned stream value:

if (!card.blockedBy().isEmpty()) {
    lines.add("");
    lines.add("Waiting for:");
    for (BlockerRef blocker : card.blockedBy()) {
        lines.add("- " + waitingBlockerText(blocker));
    }
}
if (!card.prerequisiteProblems().isEmpty()) {
    lines.add("");
    lines.add("Checklist cleanup needed:");
    for (Card.PrerequisiteProblem problem : card.prerequisiteProblems()) {
        lines.add("- " + problem.message()
                + (blank(problem.checklist()) ? "" : " Checklist: " + problem.checklist() + "."));
    }
}

Desired eval behavior

Reward classifying ordered text/list assembly as a mutation boundary where a loop can be clearer than stream forEach. Reject changing this shape to stream().toList() plus lines.addAll(...) unless the refactor materially improves readability without extra unnecessary allocation.

[martinfrancois]

Additional occurrence from a loop audit: direct one-to-one transformation into an immutable list.

Code before the prompt was executed

MarkdownTable row(Object... cells) {
    checkArgument(
            cells.length == headers.size(),
            "Markdown table row cell count must match header count: expected %s but got %s",
            headers.size(),
            cells.length);
    List<String> row = new ArrayList<>(cells.length);
    for (Object cell : cells) {
        row.add(escape(cell));
    }
    rows.add(List.copyOf(row));
    return this;
}

Prompt that caused the implementation

The maintainer asked to inspect every for loop, pass the containing method through the Streams skill, and refactor only when the stream version improves the code.

Prompt-produced code before maintainer correction

The reviewed code was the pre-prompt loop code above. There was no weaker intermediate stream implementation.

Why this belongs with the result-producing collector eval

The loop is correct, but it manually builds an intermediate list only to copy it. A stream can own the one-to-one mapping result directly because the output order is the vararg encounter order and the row list remains unmodifiable.

Maintainer-preferred code

MarkdownTable row(Object... cells) {
    checkArgument(
            cells.length == headers.size(),
            "Markdown table row cell count must match header count: expected %s but got %s",
            headers.size(),
            cells.length);
    rows.add(Arrays.stream(cells)
            .map(MarkdownTable::escape)
            .toList());
    return this;
}

Desired eval behavior

Also reward direct one-to-one loop-to-result refactors when they preserve encounter order and mutability. Reject applying this pattern when the target list must remain mutable or when the loop body performs side effects beyond producing each mapped element.