Add eval for missed Optional-helper trigger during lifecycle refactor

[martinfrancois]

Problem

A real Symphony for Trello task changed Java lifecycle code that handled an Optional<Path> workflow selector. The java-optionals skill was not invoked until a follow-up review question explicitly asked whether the Optional shape was the best one.

The previous description of this issue was incomplete. The reachable PR history shows two relevant bad states, including the isEmpty() / get() pattern the maintainer remembered.

Verified Incident Sequence

1. Initial PR state used direct Optional presence/value reads

In the earlier #434 commit from PR #450 (f848c5554487882d5b0457d013979a278ff8fd91, also present in 841407a114e2bf726d946678fc2f80e3fd4ae54b), status computed missing explicit workflow state with request.workflow().isPresent() followed by request.workflow().get():

int status(WorkerStatusRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
    List<ConnectedBoard> boards =
            selectForStatus(manifest, request.board(), request.workflow(), paths.defaultEnvPath());
    requireExistingWorkflowOrRecoverableState(paths, request.workflow(), RecoverableWorkflowState.PID);
    boards = withDefaultEnvForExplicitWorkflow(paths, request.workflow(), boards);
    boolean explicitMissingWorkflow =
            request.workflow().isPresent() && !Files.exists(request.workflow().get().toAbsolutePath().normalize());
    if (boards.isEmpty()) {
        printPidFileStatus(paths, out);
        return 0;
    }
    ManagedProcessStore store = new ManagedProcessStore(paths.stateHome());
    Set<String> duplicateBoardNames = duplicateBoardNames(boards);
    for (ConnectedBoard board : boards) {
        String boardLabel = statusBoardLabel(board, duplicateBoardNames);
        ManagedProcessStore.ManagedProcessFiles files = store.files(board.workflowPath());
        if (explicitMissingWorkflow) {
            printMissingWorkflowPidStatus(paths, store, files, boardLabel, board.workflowPath(), out);
            continue;
        }
        // ... normal status handling ...
    }
    return 0;
}

This is a direct Optional antipattern for this boundary: the code reads presence and then reads the value instead of binding the selected value once with an Optional terminal or a narrow branch at the real IO boundary.

2. The first rewritten fix still missed the Optional skill

After the maintainer asked for the missing-workflow selector behavior to be fixed, an intermediate rewrite (c47238bfb9beaf86bb7e24dfd3e5b85ba009836a) introduced a common helper, but the helper accepted Optional<Path> and used isEmpty() plus get() internally.

The three lifecycle methods also repeated the same preflight call immediately before loading the manifest:

int stop(StopWorkerRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    requireExistingExplicitWorkflow(request.workflow());
    ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
    List<ConnectedBoard> boards =
            selectForStop(manifest, request.board(), request.workflow(), paths.defaultEnvPath());
    boards = withDefaultEnvForExplicitWorkflow(paths, request.workflow(), boards);
    // ...
}

int status(WorkerStatusRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    requireExistingExplicitWorkflow(request.workflow());
    ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
    List<ConnectedBoard> boards =
            selectForStatus(manifest, request.board(), request.workflow(), paths.defaultEnvPath());
    boards = withDefaultEnvForExplicitWorkflow(paths, request.workflow(), boards);
    // ...
}

int logs(WorkerLogsRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    requireExistingExplicitWorkflow(request.workflow());
    ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
    ConnectedBoard board = selectOne(
            manifest, request.board(), request.workflow(), "logs", Optional.empty(), paths.defaultEnvPath(), false);
    // ...
}

private static void requireExistingExplicitWorkflow(Optional<Path> workflow) {
    if (workflow.isEmpty()) {
        return;
    }
    Path workflowPath = workflow.get().toAbsolutePath().normalize();
    if (!Files.exists(workflowPath)) {
        throw new TrelloBoardSetupException(
                "setup_invalid_arguments", "--workflow must point to an existing workflow file.");
    }
    validateWorkerWorkflowPath(workflowPath);
}

This is the concrete code state the eval should reproduce: the task is not merely about avoiding an Optional-only helper. It should also catch Optional.isEmpty() / Optional.get() control flow and the repeated lifecycle preflight boundary.

3. The skill was invoked only after explicit user correction

The follow-up prompt that finally caused the Optional skill to be consulted was:

regarding requireExistingExplicitWorkflow, is that really fhe best way to build this optional? check with the optional skill.

A later follow-up also pointed out the duplication:

why did yoh not build it like that in the first place? is this something we need to change in the optionals skill? also, since i caught you changing 772, 828, and 901 that all look very similar this smells like duplication we need to clean up

4. Final corrected shape

The final code moved Optional handling to the boundary and kept the domain validation helper on Path:

int stop(StopWorkerRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    ConnectedBoardManifest manifest = loadLifecycleManifest(paths, request.workflow());
    List<ConnectedBoard> boards =
            selectForStop(manifest, request.board(), request.workflow(), paths.defaultEnvPath());
    // ...
}

int status(WorkerStatusRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    ConnectedBoardManifest manifest = loadLifecycleManifest(paths, request.workflow());
    List<ConnectedBoard> boards =
            selectForStatus(manifest, request.board(), request.workflow(), paths.defaultEnvPath());
    // ...
}

int logs(WorkerLogsRequest request, PrintStream out) throws IOException {
    LocalWorkerPaths paths = LocalWorkerPaths.from(
            request.appHome(), request.configDir(), request.workspaceRoot(), request.stateHome(), environment);
    ConnectedBoardManifest manifest = loadLifecycleManifest(paths, request.workflow());
    ConnectedBoard board = selectOne(
            manifest, request.board(), request.workflow(), "logs", Optional.empty(), paths.defaultEnvPath(), false);
    // ...
}

private static ConnectedBoardManifest loadLifecycleManifest(LocalWorkerPaths paths, Optional<Path> explicitWorkflow)
        throws IOException {
    explicitWorkflow.ifPresent(LocalWorkerManager::requireExistingExplicitWorkflow);
    return new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
}

private static void requireExistingExplicitWorkflow(Path workflow) {
    Path workflowPath = workflow.toAbsolutePath().normalize();
    if (!Files.exists(workflowPath)) {
        throw new TrelloBoardSetupException(
                "setup_invalid_arguments", "--workflow must point to an existing workflow file.");
    }
    validateWorkerWorkflowPath(workflowPath);
}

Prompt That Produced The Relevant Bad State

The natural prompt did not ask for Optional help. It asked for a lifecycle bug fix, and the Optional concern appeared through the existing Java code and the requested selector boundary.

The original task prompt was much longer; this is the relevant self-contained reproduction prompt for the eval:

You are fixing PR #450.

Finding: stale managed state bypasses the missing explicit workflow contract.

For status, stop, and logs:
- When --workflow is explicitly supplied and its resolved target does not exist, reject it unconditionally.
- Do not use PID files, log files, locks, manifest-derived state, process status, or health status as a fallback source that makes the missing selector valid.
- Return CLI exit code 2.
- Emit setup_failed code=setup_invalid_arguments.
- Include exactly the actionable message --workflow must point to an existing workflow file.
- Do not include the workflow path, config directory, state directory, temp directory, log filename, state filename, account name, or other private path fragments.
- Do not write a troubleshooting report.
- Reject before any irreversible or externally visible managed-state action.

Implement strict validation at the common explicit-workflow selector boundary so it applies consistently to status, stop, and logs.

A preferred shape is:
1. Resolve and normalize the explicit path using the existing command semantics.
2. Before consulting derived PID/log state, require that the target exists.
3. If absent, throw TrelloBoardSetupException with code setup_invalid_arguments and the exact path-free message.
4. Continue using the existing regular-file validation so a directory or symlink to a directory retains --workflow must point to a regular workflow file.
5. Once an existing regular file is established, preserve the current best-effort recovery behavior for invalid or unreadable workflow contents.

Remove or rewrite all machinery that only exists to allow missing workflows through managed-state fallback.

Do not change --board or selector-free lifecycle behavior. Preserve intentional refactors and keep the change focused.

That prompt plus the code context below is enough to reproduce the missed skill-trigger case.

Proposed Eval

Add a transcript-derived reference eval, for example:

evals-reference/48-optional-helper-trigger-lifecycle-refactor/

The eval should first reproduce the without-skill miss, then verify that metadata or prompting changes make the skill trigger reliably.

Natural Eval Prompt

Do not mention $java-optionals, Optional, or skill usage in the prose prompt. The point is to test natural activation from Java code context.

Refactor this lifecycle selector validation so status, stop, and logs share the same explicit workflow preflight before loading the manifest. Preserve behavior and public messages. Keep the change small.

Edit the code below and return the revised Java snippets only.

Then include this standalone code context in the eval prompt:

import java.io.IOException;
import java.io.PrintStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.Optional;

final class LocalWorkerManager {
    int stop(StopWorkerRequest request, PrintStream out) throws IOException {
        LocalWorkerPaths paths = LocalWorkerPaths.from(request.configDir());
        requireExistingExplicitWorkflow(request.workflow());
        ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
        List<ConnectedBoard> boards = selectForStop(manifest, request.board(), request.workflow());
        return boards.size();
    }

    int status(WorkerStatusRequest request, PrintStream out) throws IOException {
        LocalWorkerPaths paths = LocalWorkerPaths.from(request.configDir());
        requireExistingExplicitWorkflow(request.workflow());
        ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
        List<ConnectedBoard> boards = selectForStatus(manifest, request.board(), request.workflow());
        return boards.size();
    }

    int logs(WorkerLogsRequest request, PrintStream out) throws IOException {
        LocalWorkerPaths paths = LocalWorkerPaths.from(request.configDir());
        requireExistingExplicitWorkflow(request.workflow());
        ConnectedBoardManifest manifest = new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
        ConnectedBoard board = selectOne(manifest, request.board(), request.workflow());
        return board.name().length();
    }

    private static void requireExistingExplicitWorkflow(Optional<Path> workflow) {
        if (workflow.isEmpty()) {
            return;
        }
        Path workflowPath = workflow.get().toAbsolutePath().normalize();
        if (!Files.exists(workflowPath)) {
            throw new TrelloBoardSetupException(
                    "setup_invalid_arguments", "--workflow must point to an existing workflow file.");
        }
        validateWorkerWorkflowPath(workflowPath);
    }

    private static void validateWorkerWorkflowPath(Path workflowPath) {
        if (Files.exists(workflowPath) && !Files.isRegularFile(workflowPath)) {
            throw new TrelloBoardSetupException(
                    "setup_invalid_arguments", "--workflow must point to a regular workflow file.");
        }
    }

    private List<ConnectedBoard> selectForStop(
            ConnectedBoardManifest manifest, Optional<String> board, Optional<Path> workflow) {
        return List.of();
    }

    private List<ConnectedBoard> selectForStatus(
            ConnectedBoardManifest manifest, Optional<String> board, Optional<Path> workflow) {
        return List.of();
    }

    private ConnectedBoard selectOne(ConnectedBoardManifest manifest, Optional<String> board, Optional<Path> workflow) {
        return new ConnectedBoard("demo");
    }

    record StopWorkerRequest(Path configDir, Optional<String> board, Optional<Path> workflow) {}
    record WorkerStatusRequest(Path configDir, Optional<String> board, Optional<Path> workflow) {}
    record WorkerLogsRequest(Path configDir, Optional<String> board, Optional<Path> workflow) {}
    record LocalWorkerPaths(Path manifestPath) {
        static LocalWorkerPaths from(Path configDir) {
            return new LocalWorkerPaths(configDir.resolve("connected-boards.json"));
        }
    }
    record ConnectedBoard(String name) {}
    record ConnectedBoardManifest(List<ConnectedBoard> boards) {}
    static final class ConnectedBoardRepository {
        ConnectedBoardRepository(Path manifestPath) {}
        ConnectedBoardManifest loadForLifecycle() throws IOException {
            return new ConnectedBoardManifest(List.of());
        }
    }
    static final class TrelloBoardSetupException extends RuntimeException {
        TrelloBoardSetupException(String code, String message) {
            super(message);
        }
    }
}

This prompt intentionally starts from the verified intermediate bad state. It exercises all three concerns from the incident:

• skill activation from Java Optional code context, not prose;
• replacing isEmpty() / get() with an appropriate Optional terminal at the boundary;
• centralizing the repeated lifecycle preflight instead of changing only one call site.

Files To Add

Create all three standard reference-eval files:

• evals-reference/48-optional-helper-trigger-lifecycle-refactor/task.md
• evals-reference/48-optional-helper-trigger-lifecycle-refactor/criteria.json
• evals-reference/48-optional-helper-trigger-lifecycle-refactor/capability.txt

task.md should contain the natural eval prompt and standalone Java code above. Do not include the expected answer, the historical commit IDs, this issue URL, or any wording that names the skill. That keeps the eval honest.

Suggested capability.txt:

Refactor Java lifecycle selector validation while naturally applying Optional helper-design guidance and preserving behavior.

Baseline Failure To Reproduce

Run the eval with the current skill metadata and record whether the agent fails to invoke/load java-optionals and returns one of these problematic shapes:

• keeps Optional.isEmpty() / Optional.get() control flow in requireExistingExplicitWorkflow;
• introduces or keeps a helper that accepts Optional<Path> only to inspect presence and then validate the contained Path;
• leaves the same explicit-workflow preflight duplicated in all three lifecycle methods;
• creates a generic Optional helper or fake Optional collection to hide the presence check;
• does not mention or apply the skill workflow despite editing Optional<Path> flow.

The historical failure included the first three bullets.

Expected Passing Behavior

The eval should pass when the agent naturally invokes java-optionals and returns a shape equivalent to:

private static ConnectedBoardManifest loadLifecycleManifest(LocalWorkerPaths paths, Optional<Path> explicitWorkflow)
        throws IOException {
    explicitWorkflow.ifPresent(LocalWorkerManager::requireExistingExplicitWorkflow);
    return new ConnectedBoardRepository(paths.manifestPath()).loadForLifecycle();
}

private static void requireExistingExplicitWorkflow(Path workflow) {
    Path workflowPath = workflow.toAbsolutePath().normalize();
    if (!Files.exists(workflowPath)) {
        throw new TrelloBoardSetupException(
                "setup_invalid_arguments", "--workflow must point to an existing workflow file.");
    }
    validateWorkerWorkflowPath(workflowPath);
}

and the three lifecycle methods call loadLifecycleManifest(paths, request.workflow()) rather than duplicating the preflight.

Suggested criteria.json Checklist

{
  "context": "The skill should naturally trigger when a Java refactor edits Optional selector flow even if the user prompt does not say Optional or name the skill.",
  "type": "weighted_checklist",
  "checklist": [
    {
      "name": "Natural Optional skill activation",
      "max_score": 12,
      "description": "The transcript shows the java-optionals skill was loaded or explicitly applied before proposing the Optional-related refactor."
    },
    {
      "name": "No isEmpty/get Optional control flow",
      "max_score": 14,
      "description": "Does not use Optional.isEmpty() followed by Optional.get() to validate the explicit workflow selector. The selected Path is bound through an appropriate Optional terminal at the boundary."
    },
    {
      "name": "No Optional-only helper parameter",
      "max_score": 12,
      "description": "Does not create or keep a helper that accepts Optional<Path> solely to inspect presence and call another operation. The domain helper that validates the workflow accepts Path."
    },
    {
      "name": "Centralizes duplicated lifecycle preflight",
      "max_score": 12,
      "description": "stop, status, and logs share one helper for explicit-workflow validation before manifest loading."
    },
    {
      "name": "Uses appropriate Optional terminal",
      "max_score": 10,
      "description": "Uses Optional.ifPresent for this side-effecting validation boundary rather than isPresent/get, isEmpty/get, orElse(null), fake Optional collections, or generic throwing helpers."
    },
    {
      "name": "Preserves behavior and messages",
      "max_score": 10,
      "description": "Keeps the missing-workflow and non-regular-workflow messages unchanged and still validates before loading the manifest."
    },
    {
      "name": "No over-engineering",
      "max_score": 8,
      "description": "Does not introduce a broad abstraction, generic Optional utility, or unrelated lifecycle rewrite."
    }
  ],
  "metadata": {
    "invocation": "natural",
    "task_type": "implementation"
  }
}

Replay Protocol

Use the repository's transcript-derived eval guidance:

1. Add the reduced reference eval using only the neutral task.md prompt and code above.
2. Run a baseline replay with the current skill metadata.
3. Confirm whether the baseline reproduces the failure: no skill activation and/or isEmpty() / get(), Optional-only helper, or duplicated preflight result.
4. Only after recording the baseline, test metadata changes.
5. Rerun the exact same natural prompt after each metadata change.
6. Promote or keep the eval only if it measures the real failure mode without leaking the intended diagnosis.

Do not include the desired answer inside task.md. Keep expected behavior only in criteria.json and maintainer docs.

Candidate Metadata Change To Test

If the baseline does not reliably trigger, try tightening the SKILL.md frontmatter description. The important part is frontmatter, because the body is loaded only after skill selection.

Current description is broad, but this exact phrase may help:

description: Write, review, and refactor Java Optional code using best practices, improving readability, and preventing common Optional antipatterns such as null-style control flow and readability regressions. Use whenever writing, reviewing, or refactoring Java code that introduces, changes, or reasons about Optional; decides whether a method should accept Optional; adds or refactors helpers with Optional parameters; handles absent, missing, nullable, fallback, or default values where Optional may be appropriate; or touches isPresent/isEmpty, get/orElseThrow, orElse(null), optional.stream(), findFirst/findAny, checked exceptions inside Optional chains, or nullable control flow.

When changing frontmatter metadata, also check whether skills/java-optionals/agents/openai.yaml needs regeneration or an equivalent metadata update according to the repo's agent metadata rules.

Validation Commands

Run the repository's standard validation for skill/eval changes:

python3 scripts/validate_skill.py skills/java-optionals
python3 scripts/validate_eval_criteria.py evals evals-reference
python3 -m py_compile scripts/validate_skill.py scripts/validate_eval_criteria.py
bash -n scripts/check_publish_dry_run.sh
tessl plugin lint .
bash scripts/check_publish_dry_run.sh .
tessl plugin publish --dry-run --bump patch .
tessl plugin publish --dry-run .

If tessl plugin publish --dry-run . fails only because the current version has already been published, record that as expected. If the skill metadata or evals should be published, bump the version before final publish dry-runs.

Acceptance Checklist

• The issue is solved without requiring access to the original Symphony for Trello repository where the miss happened.
• The issue body is self-contained enough to build the eval without reading the original transcript.
• The eval prompt is natural and does not mention $java-optionals, Optional, or the expected answer in prose outside the code sample.
• The eval can detect all failure classes from this incident: missing skill activation, isEmpty() / get() Optional control flow, Optional-only helper design, and duplicated lifecycle preflight.
• The expected passing answer keeps requireExistingExplicitWorkflow(Path) as the domain helper and uses explicitWorkflow.ifPresent(...) at the Optional boundary.
• The eval also rewards centralizing the duplicated lifecycle preflight so the agent does not only fix the Optional shape while leaving the original repeated code pattern.
• Frontmatter trigger changes are tested against the same prompt before and after the metadata change.

Notes

This issue intentionally uses a minimized Java snippet rather than requiring access to the original repository where the miss happened. The code snippets above include the relevant historical states from the incident so the eval can be reproduced independently.

[martinfrancois]

Additional occurrence: explicit catalog validation side effect

This is another missed-trigger occurrence of the same ifPresent(...) validation-boundary pattern. It targets Java 25 and is self-contained.

Code before the original prompt

Setup selected a model and reasoning effort, but did not validate an explicitly supplied effort against model-specific catalog values:

if (options.nonInteractive()) {
    return new Selection(
            TrelloBoardSetup.CodexModelDefaults.partial(model, reasoningEffort),
            modelOverride,
            reasoningEffortOverride);
}

// ... interactive selection ...

return new Selection(
        TrelloBoardSetup.CodexModelDefaults.partial(model, reasoningEffort),
        modelOverride,
        reasoningEffortOverride);

Prompt that caused the implementation

The implementation request clarified that this was not merely about adding one new effort such as xhigh. For every model, setup had to obtain the exact available reasoning levels, descriptions, order, and default from the installed Codex catalog rather than hardcoding them. Newly supplied values had to be checked against the selected model while existing workflow values and missing-catalog behavior remained pass-through.

That original prompt should have activated the Optionals skill because it introduced Optional-gated catalog validation.

Prompt-produced code before maintainer correction

The implementation correctly validated only when the CLI option was explicitly present, but expressed that side-effect boundary as a presence branch:

private static void validateExplicitReasoningEffort(
        LocalSetup.Options options,
        CodexModelSelectionDefaults selectionDefaults,
        String model,
        String reasoningEffort) {
    if (options.codexReasoningEffort().isPresent()) {
        selectionDefaults.validateReasoningEffortForModel(model, reasoningEffort);
    }
}

Later prompt that exposed the issue

The maintainer review asked:

Check with the optionals skill if it can be improved

The reviewed code was functionally correct. The later prompt exposed a small readability improvement, but should not be required to trigger the skill.

Maintainer-preferred code

private static void validateExplicitReasoningEffort(
        LocalSetup.Options options,
        CodexModelSelectionDefaults selectionDefaults,
        String model,
        String reasoningEffort) {
    options.codexReasoningEffort()
            .ifPresent(ignored ->
                    selectionDefaults.validateReasoningEffortForModel(model, reasoningEffort));
}

Behavior-equivalence analysis

Both versions skip validation when the CLI option is absent and invoke validation exactly once when it is present. They validate the same already-resolved reasoningEffort, preserve the same model lookup and accepted-value order, and propagate the same unchecked domain exception and message. No fallback, checked exception, prompt, or side-effect order changes.

The ignored Optional value is intentional: presence says the user explicitly requested an effort, while the separate argument is the normalized effective value that must be validated.

Why this should extend the eval

Reward:

• activating the Optionals skill during the original catalog-driven implementation prompt;
• using ifPresent(...) for a simple unchecked validation side-effect boundary when readable;
• preserving the resolved value and exact exception behavior;
• recognizing that predicate-only isPresent() remains appropriate elsewhere when presence itself is the returned boolean logic.

Reject rewrites that validate the raw unnormalized Optional value, run validation for absent input, hide checked work in a lambda, or replace a clear boolean-only presence predicate with boxed Optional<Boolean> flow.