Add eval for collecting final set instead of list addAll

[martinfrancois]

Problem

The Streams skill should catch a case where a stream already computes exactly the data a method returns, but the implementation first materializes a temporary list and then mutates a separate set with addAll(...).

This is eval-worthy because replacing stream().forEach(set::add) with toList() plus set.addAll(...) can still leave unnecessary intermediate state. The skill should prefer making the stream own the final collection result when mutability is not needed.

Code before the prompt was executed

The method scanned local workflow files, extracted optional server ports, collected them into a temporary list, and then copied that list into a mutable HashSet before returning it.

private Set<Integer> localWorkflowFilePortReservations(Options options, ConnectedBoard ignoredBoard) {
    Set<Integer> reserved = new HashSet<>();
    Path configDir = options.configDir();
    if (configDir == null || !Files.isDirectory(configDir)) {
        return reserved;
    }
    try (var files = Files.list(configDir)) {
        List<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)
                .toList();
        reserved.addAll(workflowPorts);
    } catch (IOException ignored) {
        // A config directory that cannot be listed leaves only the manifest and probe checks.
    }
    return reserved;
}

Prompt that caused the implementation

The earlier cleanup prompt asked for repeated Java Streams and Optional cleanup passes across the codebase, including replacing stream mutation patterns and checking for remaining stream antipatterns. The implementation focused on removing direct forEach(...) mutation, but still left this temporary-list-then-addAll(...) shape.

Later prompt that exposed the issue

A review comment asked:

Is there really no better way here? Use the streams skill

The comment pointed at reserved.addAll(workflowPorts); after workflowPorts had just been produced by a stream.

Prompt-produced code before maintainer correction

Same as the code before the prompt: the stream produced workflowPorts, then reserved.addAll(workflowPorts) copied it into the returned set.

Why the prompt-produced code is weak

The code is functionally correct, but the temporary list is unnecessary:

1. The method returns the collected ports directly.
2. The returned collection does not need to be mutable.
3. The list allocation does not express the result type; it is only an intermediate copy source.
4. Replacing external mutation with toList() is incomplete when the next line immediately mutates another collection with the list.

Behavior-equivalence analysis

The final code preserves behavior:

• Missing or non-directory config dirs still return an empty set.
• Unreadable config dirs still return an empty set.
• Duplicate ports are still collapsed because the method returns a set.
• The caller only merges the returned ports into another set, so the returned collection does not need to be mutable.
• Encounter order is not part of the contract because the result is used for membership checks.

Maintainer-preferred code

private Set<Integer> localWorkflowFilePortReservations(Options options, ConnectedBoard ignoredBoard) {
    Path configDir = options.configDir();
    if (configDir == null || !Files.isDirectory(configDir)) {
        return Set.of();
    }
    try (var files = Files.list(configDir)) {
        return 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(toImmutableSet());
    } catch (IOException ignored) {
        // A config directory that cannot be listed leaves only the manifest and probe checks.
        return Set.of();
    }
}

Why the replacement is better

The stream now owns the final result. There is no temporary list and no extra mutable set inside the helper. toImmutableSet() makes it explicit that callers should not mutate the helper result, while still preserving the membership semantics the caller needs.

Desired eval behavior

• Reward detecting stream output that is immediately copied into another collection with addAll(...).
• Reward checking whether the returned collection must be mutable before choosing the collector.
• Reward producing the final collection directly from the stream when behavior is preserved.
• Reward explaining that toList() plus addAll(...) may still be weaker than collecting directly to the needed result type.
• Reward preserving exception and empty-directory behavior around Files.list(...).

Anti-patterns the eval should reject

• Replacing addAll(...) with stream().forEach(set::add).
• Keeping toList() only to immediately copy it into a returned collection.
• Using Collectors.toSet() in a codebase where the result mutability is intentionally avoided or where lint forbids ambiguous JDK collectors.
• Changing IO exception handling or probing behavior while doing the stream cleanup.

Suggested eval name

collect-final-set-instead-of-list-addall

[martinfrancois]

Additional occurrence from TrelloClient.fetchTerminalCards(...) where the same eval should cover the opposite outcome: sometimes the correct answer is to keep the explicit append loop instead of replacing it with toList() plus addAll(...).

Code before the attempted refactor

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);
    }
}

Prompt that caused the attempted implementation

A review pass asked whether there was a better way around the explicit append loop in fetchTerminalCards(...). A previously recovered follow-up also warned that a forEach(...) or loop changed to addAll(...) can be worse when it creates an unnecessary intermediate object.

Prompt-produced code before correction

The attempted stream-looking implementation made each archived-list response allocate a temporary list, then copied it into the mutable aggregate:

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

private List<Card> terminalCards(
        List<Map<String, Object>> payloads, BoardContext context, EffectiveConfig config) {
    return payloads.stream()
            .map(payload -> normalize(payload, context, config))
            .flatMap(Optional::stream)
            .filter(card -> isTerminal(card, config))
            .toList();
}

Why this is worse

The refactor is functionally correct, but it is not an improvement:

1. fetchTerminalCards(...) already has an explicit Trello I/O loop over archived lists.
2. The aggregate normalized list must stay mutable because board-level results are combined with archived-list results before de-duplication.
3. The attempted helper allocates one extra list per archived Trello list only to copy it immediately into normalized.
4. The stream shape hides that the method is appending into one cross-response aggregate.

Final preferred code

The final code keeps the explicit append helper:

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

The skill should not reward replacing an explicit append loop with toList() plus addAll(...) when the caller still needs one mutable aggregate. It should either keep the behavior-preserving append loop or propose a broader design only if it avoids both hidden side effects and extra per-batch list allocation.

[martinfrancois]

Additional occurrence from AdrConformanceTest.assertUniqueAdrNumbers(...) where the same eval should reject toList() plus addAll(...) as a cosmetic replacement for stream mutation.

Code before the prompt was executed

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())));

Prompt-produced code before correction

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 this is weak

The stream result is not the method result. It is only a temporary list copied into the existing violation accumulator, so it adds allocation without removing the accumulator boundary.

Final preferred code

for (Map.Entry<String, List<Path>> entry : filesByNumber.entrySet()) {
    if (entry.getValue().size() > 1) {
        violations.add("ADR number %s is used by multiple files: %s".formatted(entry.getKey(), entry.getValue()));
    }
}

Desired eval behavior

When the target API is already an accumulator supplied by the caller, the skill should not replace stream forEach(accumulator::add) with toList() plus accumulator.addAll(...). It should either return the stream-owned result from the method, or keep a direct loop when the method's contract is to append into an existing accumulator.

[martinfrancois]

Additional occurrence from a loop audit: flatten vararg groups directly instead of addAll into a temporary list.

Code before the prompt was executed

@SafeVarargs
private static List<String> optionNames(List<String>... groups) {
    List<String> names = new ArrayList<>();
    for (List<String> group : groups) {
        names.addAll(group);
    }
    return List.copyOf(names);
}

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 final-result collection eval

This is the safe version of the earlier toList() plus addAll(...) concern: the stream collects directly to the final returned list. There is no later mutation, and Stream.toList() preserves the unmodifiable result shape of List.copyOf(...).

Maintainer-preferred code

@SafeVarargs
private static List<String> optionNames(List<String>... groups) {
    return Arrays.stream(groups)
            .flatMap(List::stream)
            .toList();
}

Desired eval behavior

Reward flattening nested collections directly into the final result when the final result is immutable and no side effects are hidden. Continue rejecting rewrites that allocate toList() only to immediately addAll(...) into another collection.

[martinfrancois]

Additional occurrence: cleanup list extension should use a side-effect boundary

This is another instance of the same collect to a temporary list, then addAll pattern, but the correct fix is different from the production immutable-set case. Here the method intentionally owns a mutable cleanup list, so the stream should feed that side-effect boundary directly instead of materializing a temporary list only to copy it.

Prompt/review that exposed the issue

A PR review comment asked:

What do you mean? Shouldn't it be forEach here?

The earlier agent response had defended addAll(...), but the better distinction is: addAll(...) is fine when adding an existing collection, but it is weaker when the collection exists only because a stream was materialized for the sole purpose of being copied into another mutable collection.

Prompt-produced code before maintainer correction

List<String> boardIds = new ArrayList<>(disposableBoardIds);
List<Throwable> failures = new ArrayList<>();
try {
    List<String> openDisposableBoardIds = trello.openBoardIdsByNamePrefix(workspaceId, runId).stream()
            .filter(boardId -> !boardIds.contains(boardId))
            .toList();
    boardIds.addAll(openDisposableBoardIds);
} catch (AssertionError | RuntimeException e) {
    failures.add(e);
}

Why the prompt-produced code is weaker

The temporary openDisposableBoardIds list has no independent meaning. It is allocated only so the next statement can copy its contents into boardIds. That makes the stream look result-producing even though the method's real operation is extending a cleanup accumulator.

Behavior-equivalence analysis

The final code preserves encounter order and keeps the same duplicate filtering check against the already known boardIds. It does not change exception handling: failures from fetching or streaming discovered boards still land in the same catch block.

Maintainer-preferred code

List<String> boardIds = new ArrayList<>(disposableBoardIds);
List<Throwable> failures = new ArrayList<>();
try {
    trello.openBoardIdsByNamePrefix(workspaceId, runId).stream()
            .filter(boardId -> !boardIds.contains(boardId))
            .forEach(boardIds::add);
} catch (AssertionError | RuntimeException e) {
    failures.add(e);
}

Desired eval behavior

• Reward recognizing when a mutable accumulator is the honest owner of the method state.
• Reward forEach(accumulator::add) at an explicit side-effect boundary when a temporary stream result would only be copied with addAll(...).
• Still reject replacing a stream result with forEach(...) when a result-producing immutable collector would be clearer and behavior-preserving.