feat: add eval for side-effecting Optional branches

[martinfrancois]

Problem

Add an eval that teaches the Java Optionals skill to prevent and repair Optional.isPresent() plus orElseThrow() control flow when a Java 9+ side-effecting present/empty branch should use Optional.ifPresentOrElse(...).

This issue covers two related failures:

1. Introduction-time failure: the weaker Optional shape was introduced while implementing a new lookup branch, even though the Java Optionals skill was available in the session and its trigger description covers writing or reviewing code that touches isPresent, orElseThrow, absent/missing values, or Optional control flow.
2. Review-time repair: the same weaker shape was later noticed and refactored after the user explicitly asked whether if (request.lookup().isPresent()) was best practice.

This is eval-worthy because the weaker code is functionally correct, but it uses null-style Optional control flow: isPresent() gates the branch and orElseThrow() retrieves the same Optional value afterward. The skill should learn to trigger before introducing that shape, not only after a user explicitly calls it out.

Area

Evals or scoring

Code before the feature prompt was executed

Before lookup support existed, the method always rendered the full private diagnostics context. There was no present/empty lookup branch yet:

private String renderPrivateContext(DiagnosticsRequest request, Optional<DiagnosticsTokenHasher> sharedTokenHasher)
        throws IOException {
    DiagnosticsContext context = diagnosticsContext(request, sharedTokenHasher);

    StringBuilder body = new StringBuilder();
    body.append("# Symphony for Trello Private Context\n\n");
    body.append("Private diagnostics context. Do not paste this output into public issues.\n");
    body.append("It may include Trello board names, board ids, board URLs, and local paths.\n");
    body.append("It does not include credential values or worker log contents.\n");

    section(body, "Command");
    line(body, "time_utc", now().toString());
    line(body, "command", String.join(" ", diagnosticsArguments(request, true)));
    line(body, "command_context", commandContext());
    appendSelectionMetadata(body, context);
    appendDiagnosticsTokenKeyStatus(body);

    section(body, "Local Paths");
    appendLocalPathIdentifiers(body, context.paths(), context.manifestPath());

    section(body, "Connected Board Identifiers");
    appendLocalManifestIdentifiers(
            body,
            context.selectedManifest(),
            context.manifestSnapshot().status(),
            context.paths().defaultEnvPath());

    section(body, "Workflow Identifiers");
    appendLocalWorkflowIdentifiers(body, context.selectedWorkflowPaths());

    section(body, "Log Identifiers");
    appendLocalLogIdentifiers(
            body, context.paths().stateHome(), context.selectedWorkflowPaths(), context.selected());

    return body.toString();
}

Feature prompt that caused isPresent() to be introduced

The implementation prompt asked for a focused private diagnostics token lookup feature. Relevant requirements included:

Keep the PR as a focused private diagnostics token lookup feature.

Generated lookup commands and executable docs do not quote concrete <path:...> tokens.

Keep existing lookup tests for found token, managed PID token, file-backed secret token, missing token without full private dump, malformed token without arbitrary private search, and --lookup rejected without --show-private-context.

The implementation needed a new request.lookup() branch: when lookup is present, render only the lookup result; when absent, render the full private diagnostics context.

Prompt-produced code that introduced the weaker Optional shape

The feature implementation added the branch using isPresent() and orElseThrow():

private String renderPrivateContext(DiagnosticsRequest request, Optional<DiagnosticsTokenHasher> sharedTokenHasher)
        throws IOException {
    DiagnosticsContext context = diagnosticsContext(request, sharedTokenHasher);

    StringBuilder body = new StringBuilder();
    body.append("# Symphony for Trello Private Context\n\n");
    body.append("Private diagnostics context. Do not paste this output into public issues.\n");
    body.append("It may include Trello board names, board ids, board URLs, and local paths.\n");
    body.append("It does not include credential values or worker log contents.\n");

    section(body, "Command");
    line(body, "time_utc", now().toString());
    line(body, "command", String.join(" ", diagnosticsArguments(request, true)));
    line(body, "command_context", commandContext());
    appendSelectionMetadata(body, context);
    appendDiagnosticsTokenKeyStatus(body);
    if (request.lookup().isPresent()) {
        section(body, "Lookup");
        appendPrivateContextLookup(body, context, request.lookup().orElseThrow());
        return body.toString();
    }

    section(body, "Local Paths");
    appendLocalPathIdentifiers(body, context.paths(), context.manifestPath());

    section(body, "Connected Board Identifiers");
    appendLocalManifestIdentifiers(
            body,
            context.selectedManifest(),
            context.manifestSnapshot().status(),
            context.paths().defaultEnvPath());

    section(body, "Workflow Identifiers");
    appendLocalWorkflowIdentifiers(body, context.selectedWorkflowPaths());
    appendLocalSecretFileIdentifiers(body, context.selectedWorkflowPaths());

    section(body, "Log Identifiers");
    appendLocalLogIdentifiers(
            body, context.paths().stateHome(), context.selectedWorkflowPaths(), context.selected());

    section(body, "Process State Identifiers");
    appendLocalProcessStateIdentifiers(
            body, context.paths().stateHome(), context.selectedWorkflowPaths(), context.selected());

    return body.toString();
}

Why the prompt-produced code is weak

1. It reads the same Optional twice.
   request.lookup().isPresent() decides the branch, then request.lookup().orElseThrow() retrieves the value. That is safe in this code, but it is the same null-check-then-get shape Optional is meant to avoid.

2. It uses an exception-oriented accessor for a value already proven present.
   orElseThrow() communicates failure handling, but this branch is not a failure path. It is ordinary present-value control flow.

3. It splits one present/empty decision across an early return and the rest of the method.
   The method has exactly two side-effecting rendering branches: render a lookup result when the Optional is present, or render the full private context when it is absent. That maps directly to ifPresentOrElse(...).

4. It shows a skill-trigger gap.
   The Java Optionals skill description says to use it when writing, reviewing, or refactoring Optional code, and specifically when touching isPresent, orElseThrow, absent/missing values, or nullable control flow. The feature prompt did not explicitly mention Optional style, so an eval should ensure the skill still triggers based on the code being introduced.

Follow-up prompt that caused the repair

The user later asked:

in if (request.lookup().isPresent()) { can you please use the java optionals skill to verify if that is best practice in that part of the code?

The Java baseline for this code was Java 25, so Java 9+ Optional APIs such as ifPresentOrElse(...) were available.

Maintainer-preferred code

private String renderPrivateContext(DiagnosticsRequest request, Optional<DiagnosticsTokenHasher> sharedTokenHasher)
        throws IOException {
    DiagnosticsContext context = diagnosticsContext(request, sharedTokenHasher);

    StringBuilder body = new StringBuilder();
    body.append("# Symphony for Trello Private Context\n\n");
    body.append("Private diagnostics context. Do not paste this output into public issues.\n");
    body.append("It may include Trello board names, board ids, board URLs, and local paths.\n");
    body.append("It does not include credential values or worker log contents.\n");

    section(body, "Command");
    line(body, "time_utc", now().toString());
    line(body, "command", String.join(" ", diagnosticsArguments(request, true)));
    line(body, "command_context", commandContext());
    appendSelectionMetadata(body, context);
    appendDiagnosticsTokenKeyStatus(body);
    request.lookup()
            .ifPresentOrElse(
                    lookup -> {
                        section(body, "Lookup");
                        appendPrivateContextLookup(body, context, lookup);
                    },
                    () -> {
                        section(body, "Local Paths");
                        appendLocalPathIdentifiers(body, context.paths(), context.manifestPath());

                        section(body, "Connected Board Identifiers");
                        appendLocalManifestIdentifiers(
                                body,
                                context.selectedManifest(),
                                context.manifestSnapshot().status(),
                                context.paths().defaultEnvPath());

                        section(body, "Workflow Identifiers");
                        appendLocalWorkflowIdentifiers(body, context.selectedWorkflowPaths());
                        appendLocalSecretFileIdentifiers(body, context.selectedWorkflowPaths());

                        section(body, "Log Identifiers");
                        appendLocalLogIdentifiers(
                                body,
                                context.paths().stateHome(),
                                context.selectedWorkflowPaths(),
                                context.selected());

                        section(body, "Process State Identifiers");
                        appendLocalProcessStateIdentifiers(
                                body,
                                context.paths().stateHome(),
                                context.selectedWorkflowPaths(),
                                context.selected());
                    });

    return body.toString();
}

Why the replacement is better

The replacement consumes the Optional once and names the present value directly in the present branch. It makes the method's two outcomes explicit: present lookup renders only lookup output; empty lookup renders the full private context.

This is also a better use of Optional because ifPresentOrElse(...) is designed for side-effecting present/empty branches. It avoids a fake transformation chain, avoids orElse(null), avoids a generic helper, and preserves the original output ordering and behavior.

The eval should not blindly require Optional chains everywhere. It should prefer this shape only when the branch is ordinary Optional control flow and the target Java version supports ifPresentOrElse(...).

Desired eval behavior

• In an implementation task, trigger the Java Optionals skill when the proposed Java code introduces an Optional branch, isPresent, orElseThrow, absent/missing lookup handling, or similar Optional control flow, even when the user did not explicitly ask for Optional advice.
• Reward implementing the new lookup branch with ifPresentOrElse(...) from the start when both present and empty branches are side-effecting and behavior can be preserved.
• In a review/refactor task, reward recognizing isPresent() plus get() / orElseThrow() as a weaker Optional style when the value is immediately consumed in a side-effecting branch.
• Reward explaining that the original code is functionally correct but less idiomatic and less direct.
• Reward preserving ordering, side effects, checked exceptions, and return behavior.
• Do not require the refactor when ifPresentOrElse(...) is unavailable due to Java baseline.
• Do not require the refactor when checked exceptions, complex returns, or clearer imperative flow make an ordinary if more readable.

Anti-patterns the eval should reject

• Introducing optional.isPresent() plus optional.get() or optional.orElseThrow() for this simple present/empty side-effecting branch.
• Keeping that shape during review when Java 9+ ifPresentOrElse(...) is available and clearer.
• Replacing the branch with orElse(null) followed by a null check.
• Building a generic Optional helper only to avoid ifPresentOrElse(...).
• Using map(...), orElseGet(...), or stream() in a way that hides side effects or changes execution order.
• Creating an intermediate collection or fake stream pipeline for one Optional value.
• Changing the output by rendering both lookup and full-context sections.

Suggested eval name

side-effecting-ifpresentorelse-lookup

Alternatives considered

A separate issue for the introduction-time trigger would duplicate the same code, preferred replacement, and scoring rule. Keeping both phases in one issue makes the desired eval clearer: the skill should prevent the antipattern during implementation and catch it during review.

A broader eval about all isPresent() usage would be too noisy because some imperative Optional checks are acceptable.

Current workaround

A reviewer can manually ask the agent to revisit isPresent() / orElseThrow() pairs with the Java Optionals skill, but the skill should learn to catch this pattern during the first implementation or review pass.

Additional context

The example is intentionally self-contained. It does not require access to the source repository or pull request to understand the Optional lesson.

AI Disclosure: This issue was prepared with AI assistance. The human contributor requested the eval capture and reviewed the intended behavior.

[martinfrancois]

Additional checked-boundary occurrence from a later cleanup pass.

Prompt that caused the cleanup

A maintainer asked for repeated Optional and Stream cleanup passes until the actionable pass was 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

Code before cleanup

Several checked prompt/IO boundaries already used an empty guard, but then reopened the Optional with orElseThrow() just to read the present value:

Optional<String> configuredWorkspaceId = options.workspaceId();
if (configuredWorkspaceId.isEmpty()) {
    return chooseWorkspaceId(workspaces, terminal, options.nonInteractive());
}
return configuredWorkspaceId.orElseThrow();

if (options.serverPort().isEmpty()) {
    return selectedImportServerPort(options, manifest, workflowPath, reservedPorts);
}
return validatedRequestedServerPort(
        options, manifest, workflowPath, options.serverPort().orElseThrow(), reservedPorts);

Optional<Integer> existingPort = workflowConfig
        .serverPort(workflowPath, name -> LocalEnvironment.get(name, options.envPath()))
        .filter(port -> port != 0)
        .filter(port -> !reservedPorts.contains(port));
if (existingPort.isEmpty()) {
    return Optional.empty();
}
int port = existingPort.orElseThrow();

Final code for this occurrence

Optional<String> configuredWorkspaceId = options.workspaceId();
if (configuredWorkspaceId.isEmpty()) {
    return chooseWorkspaceId(workspaces, terminal, options.nonInteractive());
}
return configuredWorkspaceId.get();

Optional<Integer> serverPort = options.serverPort();
if (serverPort.isEmpty()) {
    return selectedImportServerPort(options, manifest, workflowPath, reservedPorts);
}
return validatedRequestedServerPort(
        options, manifest, workflowPath, serverPort.get(), reservedPorts);

Optional<Integer> existingPort = workflowConfig
        .serverPort(workflowPath, name -> LocalEnvironment.get(name, options.envPath()))
        .filter(port -> port != 0)
        .filter(port -> !reservedPorts.contains(port));
if (existingPort.isEmpty()) {
    return Optional.empty();
}
int port = existingPort.get();

Sibling setup selector methods used the same cleanup: after the checked fallback branch, read the present selector with get().

Why this belongs on the same eval family

The skill should distinguish ordinary value flow from checked prompt/IO boundaries. At a checked boundary, a plain empty guard is acceptable, but the present read should be local and direct. orElseThrow() after the empty guard adds noise and suggests absence is still possible after the branch handled it.

The eval should reward if (value.isEmpty()) { checkedFallback(); } return value.get(); for checked boundaries, while still rejecting this shape for ordinary value transformations where map(...), orElse(...), or orElseGet(...) is clearer.

[martinfrancois]

Additional Optional cleanup occurrence from a later full-codebase pass.

Prompt that caused the cleanup

A maintainer asked for repeated Optional and Stream cleanup passes until no actionable findings remained:

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

This should trigger the Optionals skill because the audit touched orElse(null), isPresent/isEmpty, get, checked prompt boundaries, and Optional default handling.

Code before cleanup

The first method used isPresent() followed by orElseThrow() for ordinary Optional value flow:

private static Optional<Path> preflightWorkflowPath(LocalSetup.Options options) {
    if (options.workflowPathExplicit()) {
        return Optional.of(options.workflowPath());
    }
    if (options.existingBoardId().isPresent()) {
        return Optional.empty();
    }
    if (options.boardName().isPresent()) {
        return Optional.of(resolveWorkflowPath(options, options.boardName().orElseThrow()));
    }
    if (options.nonInteractive()) {
        return Optional.of(resolveWorkflowPath(options, DEFAULT_BOARD_NAME));
    }
    return Optional.empty();
}

A setup-normalization method temporarily converted absent paths to null, then immediately branched on those nulls to restore defaults:

Path manifest = request.manifestPath().orElse(null);
boolean manifestPathExplicit = request.manifestPath().isPresent();
Path envPath = request.envPath().orElse(null);
// ...
configDir = configDir.toAbsolutePath().normalize();
envPath = resolveUserDataPath(envPath == null ? DEFAULT_ENV_PATH : envPath, configDir);
manifest = resolveUserDataPath(manifest == null ? Path.of("connected-boards.json") : manifest, configDir);

A test asserted presence, then reopened the Optional values with get():

assertThat(first).isPresent();
assertThat(second).isPresent();
assertThat(second.get()).isNotEqualTo(first.get());
assertThat(first.get()).exists();
assertThat(second.get()).exists();

Final code

private static Optional<Path> preflightWorkflowPath(LocalSetup.Options options) {
    if (options.workflowPathExplicit()) {
        return Optional.of(options.workflowPath());
    }
    if (options.existingBoardId().isPresent()) {
        return Optional.empty();
    }
    return options.boardName()
            .map(name -> resolveWorkflowPath(options, name))
            .or(() -> options.nonInteractive()
                    ? Optional.of(resolveWorkflowPath(options, DEFAULT_BOARD_NAME))
                    : Optional.empty());
}

Path manifest = request.manifestPath().orElse(Path.of("connected-boards.json"));
boolean manifestPathExplicit = request.manifestPath().isPresent();
Path envPath = request.envPath().orElse(DEFAULT_ENV_PATH);
// ...
configDir = configDir.toAbsolutePath().normalize();
envPath = resolveUserDataPath(envPath, configDir);
manifest = resolveUserDataPath(manifest, configDir);

assertThat(first).hasValueSatisfying(path -> assertThat(path).exists());
assertThat(second).hasValueSatisfying(path -> assertThat(path).exists());
assertThat(second).isNotEqualTo(first);

Why this belongs on the same eval family

This is the same lesson as the existing issue: the weaker code is functionally correct, but it uses Optional as a null/presence gate and then reopens the value. The skill should reward keeping the value inside Optional operations for ordinary value flow and using AssertJ Optional assertions in tests.

The cleanup also preserves the important exception-boundary distinction. Other nearby isEmpty() plus get() branches were intentionally left alone because their empty branches perform checked terminal prompting or checked file/process IO. The eval should not blindly reject those checked-boundary branches; it should reject the ordinary isPresent()/orElseThrow() and orElse(null)-then-null-branch shapes shown above.

Desired eval behavior

• Trigger during full-codebase Optional cleanup requests, even when the prompt also mentions Streams.
• Reward replacing ordinary isPresent() plus value read with map(...)/or(...) when that preserves behavior.
• Reward replacing orElse(null) plus immediate null/default branching with a direct Optional default.
• Reward AssertJ Optional assertions that avoid optional.get() in tests.
• Preserve plain checked-prompt or checked-IO empty guards when Java checked exceptions make fluent Optional APIs less readable.

[martinfrancois]

Additional ordinary value-flow occurrence from the same cleanup sweep.

Prompt that caused the cleanup

The maintainer asked for a broad Optional scan and specifically called out isEmpty followed by value reads:

please do a broad scan of isEmpty + get and then use the optional skill on all of them tonsee if you can improve them

Code before cleanup

This method first filtered an optional configured port, then used isEmpty() and get() to read it for ordinary value flow:

Optional<Integer> existingPort = workflowConfig
        .serverPort(workflowPath, name -> LocalEnvironment.get(name, options.envPath()))
        .filter(port -> port != 0)
        .filter(port -> !reservedPorts.contains(port));
if (existingPort.isEmpty()) {
    return Optional.empty();
}
int port = existingPort.get();
if (!boardSetup.portInUse(port)) {
    return Optional.of(port);
}
if (canStopManagedWorkflow(options, manifest, workflowPath, port)) {
    return Optional.of(port);
}
throw new TrelloBoardSetupException(
        "setup_server_port_conflict", "--server-port %d is already in use on 127.0.0.1.".formatted(port));

Why the old code is weak

This is not a checked prompt or checked-I/O fallback boundary. Once the optional port is present, the code performs an ordinary transformation from Optional<Integer> to Optional<Integer> while possibly throwing a domain exception. The empty guard plus get() makes the Optional behave like nullable state.

Maintainer-preferred code

Bind the present value once with map(...) and name the domain validation in a helper:

Optional<Integer> existingPort = workflowConfig
        .serverPort(workflowPath, name -> LocalEnvironment.get(name, options.envPath()))
        .filter(port -> port != 0)
        .filter(port -> !reservedPorts.contains(port));
return existingPort.map(port -> replaceableExistingPort(options, manifest, workflowPath, port));

private int replaceableExistingPort(
        LocalSetup.Options options, ConnectedBoardManifest manifest, Path workflowPath, int port) {
    if (boardSetup.portInUse(port) && !canStopManagedWorkflow(options, manifest, workflowPath, port)) {
        throw new TrelloBoardSetupException(
                "setup_server_port_conflict", "--server-port %d is already in use on 127.0.0.1.".formatted(port));
    }
    return port;
}

Behavior-equivalence analysis

The refactor preserves laziness for the validation work: replaceableExistingPort(...) runs only when existingPort is present. It preserves the returned Optional.empty() when no existing port is usable, preserves the same conflict exception, and does not change checked-exception behavior.

Desired eval behavior

Reward distinguishing this ordinary value-flow case from the checked prompt/IO empty guards already covered in this issue. The skill should allow empty-guard get() at checked boundaries, but prefer map(...) when the present branch is a normal value transformation with domain validation.