ronantakizawa/codereview-bench huggingface dataset:
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/runners/AclChecker.java (Java)

Code before:
package io.kestra.core.runners;

import io.kestra.core.services.FlowService;

import java.util.List;
import java.util.Objects;

public interface AclChecker {

    void checkAllowAllNamespace();

    void checkAllowNamespace(String namespace);

    void checkAllowNamespaces(List<String> namespaces);

    void checkAllowResources(AllowedResources allowedResources);

    interface AllowedResources {
        void checkAllowed(RunContext.FlowInfo flowInfo);
    }

    class AllowAllNamespace implements AllowedResources {
        private final FlowService flowService;

        AllowAllNamespace(FlowService flowService) {
            this.flowService = Objects.requireNonNull(flowService);
        }

        @Override
        public void checkAllowed(RunContext.FlowInfo flowInfo) {
            this.flowService.checkAllowedAllNamespaces(flowInfo.tenantId(), flowInfo.tenantId(), flowInfo.namespace());
        }
    }

    class AllowNamespaces implements AllowedResources {
        private final FlowService flowService;
        private final List<String> namespaces;

        AllowNamespaces(FlowService flowService, List<String> namespaces) {
            this.flowService = Objects.requireNonNull(flowService);
            this.namespaces = Objects.requireNonNull(namespaces);
            if (namespaces.isEmpty()) {
                throw new IllegalArgumentException("At least one namespace must be provided");
            }

Review comment (suggestion):
With my above proposal this becomes : 

```
interface AllowedResources {
        void check();
}
```

Code after:
package io.kestra.core.runners;

import java.util.List;

/**
 * Check if the current taskrun has access to the requested resources.
 *
 * <p>
 * IMPORTANT: remember to call the <code>check()</code> method to check the ACL.
 *
 * @see AllowedResources
 */
public interface AclChecker {

    /**Tasks that need to access resources outside their namespace should use this interface to check ACL (Allowed namespaces in EE).
     * Allow all namespaces.
     * <p>
     * IMPORTANT: remember to call the <code>check()</code> method to check the ACL.
     */
    AllowedResources allowAllNamespaces();

    /**
     * Allow only the given namespace.
     * <p>
     * IMPORTANT: remember to call the <code>check()</code> method to check the ACL.
     */
    AllowedResources allowNamespace(String namespace);

    /**
     * Allow only the given namespaces.
     * <p>
     * IMPORTANT: remember to call the <code>check()</code> method to check the ACL.
     */
    AllowedResources allowNamespaces(List<String> namespaces);

    /**
     * Represents a set of allowed resources.
     * Tasks that need to access resources outside their namespace should call the <code>check()</code> method to check the ACL (Allowed namespaces in EE).
     */
    interface AllowedResources {
        /**
         * Check if the current taskrun has access to the requested resources.
         */
        void check();
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/models/triggers/AbstractTrigger.java (Java)

Code before:
        implementation = Object.class, oneOf = {List.class, Map.class}
    )
    @JsonSerialize(using = ListOrMapOfLabelSerializer.class)
    @JsonDeserialize(using = ListOrMapOfLabelDeserializer.class)
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private List<@NoSystemLabelValidation Label> labels;

    @PluginProperty(group = PluginProperty.CORE_GROUP)
    @Schema(
        title = "List of execution states after which a trigger should be stopped (a.k.a. disabled)."
    )
    private List<State.Type> stopAfter;

    @Builder.Default
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private boolean logToFile = false;

    @Builder.Default
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private boolean failOnTriggerError = false;

    /**
     * For backward compatibility: we rename minLogLevel to logLevel.
     * @deprecated use {@link #logLevel} instead
     */
    @Deprecated
    public void setMinLogLevel(Level minLogLevel) {
        this.logLevel = minLogLevel;
    }
}


Review comment (suggestion):
Hi @fhussonnois should this be of type `Boolean` instead? Getting an error when trying to add `allowConcurrent` to my Flow trigger.

Code after:
        implementation = Object.class, oneOf = {List.class, Map.class}
    )
    @JsonSerialize(using = ListOrMapOfLabelSerializer.class)
    @JsonDeserialize(using = ListOrMapOfLabelDeserializer.class)
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private List<@NoSystemLabelValidation Label> labels;

    @PluginProperty(group = PluginProperty.CORE_GROUP)
    @Schema(
        title = "List of execution states after which a trigger should be stopped (a.k.a. disabled)."
    )
    private List<State.Type> stopAfter;

    @Builder.Default
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private boolean logToFile = false;

    @Builder.Default
    @PluginProperty(hidden = true, group = PluginProperty.CORE_GROUP)
    private boolean failOnTriggerError = false;

    @PluginProperty(group = PluginProperty.CORE_GROUP)
    @Schema(
        title = "Specifies whether a trigger is allowed to start a new execution even if a previous run is still in progress."
    )
    private boolean allowConcurrent = false;

    /**
     * For backward compatibility: we rename minLogLevel to logLevel.
     * @deprecated use {@link #logLevel} instead
     */
    @Deprecated
    public void setMinLogLevel(Level minLogLevel) {
        this.logLevel = minLogLevel;
    }
}
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/exceptions/InputOutputValidationException.java (Java)

Code before:
package io.kestra.core.exceptions;

import io.kestra.core.models.flows.Data;
import io.kestra.core.models.flows.Output;

/**
 * Exception that can be thrown when Inputs/Outputs have validation problems.
 */
public class InputOutputValidationException extends KestraRuntimeException {
    public InputOutputValidationException(String message) {
        super(message);
    }

    public static InputOutputValidationException of( String message, Data data){
        String inputOutputMessage = "Invalid value for " + (data instanceof Output ? "output" : "input") + " `" + data.getId() + "`. Cause: " + message;
        return new InputOutputValidationException(inputOutputMessage);
    }
    public static InputOutputValidationException of(String message){
        return new InputOutputValidationException(message);
    }

}


Review comment (suggestion):
Could you create two methods here to avoid the if/insteaceof :
* `of(String message, Input input)`
* `of(String message, Output input)`



Code after:
package io.kestra.core.exceptions;

import io.kestra.core.models.flows.Data;
import io.kestra.core.models.flows.Input;
import io.kestra.core.models.flows.Output;

import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

/**
 * Exception that can be thrown when Inputs/Outputs have validation problems.
 */
public class InputOutputValidationException extends KestraRuntimeException {
    public InputOutputValidationException(String message) {
        super(message);
    }
    public static InputOutputValidationException of( String message, Input<?> input){
        String inputMessage = "Invalid value for input" + " `" + input.getId() + "`. Cause: " + message;
        return new InputOutputValidationException(inputMessage);
    }
    public static InputOutputValidationException of( String message, Output output){
        String outputMessage = "Invalid value for output" + " `" + output.getId() + "`. Cause: " + message;
        return new InputOutputValidationException(outputMessage);
    }
    public static InputOutputValidationException of(String message){
        return new InputOutputValidationException(message);
    }

    public static InputOutputValidationException merge(Set<InputOutputValidationException> exceptions){
        String combinedMessage = exceptions.stream()
                .map(InputOutputValidationException::getMessage)
                .collect(Collectors.joining(System.lineSeparator()));
        throw new InputOutputValidationException(combinedMessage);
    }

}
Repository: kestra-io/kestra
File: ui/src/components/inputs/FileExplorer.vue (Vue)

Code before:
        z-index: 9999;
        border: 1px solid var(--ks-border-primary);

        & li {
            height: 30px;
            padding: 16px;
            font-size: var(--el-font-size-small);
            color: var(--ks-content-primary);

            &:hover {
                color: var(--ks-content-secondary);
            }
        }
    }

    :deep(.el-tree) {
        height: calc(100% - 64px);
        overflow: auto;
        background: var(--ks-background-panel);

        .el-tree__empty-block {
            height: auto;
        }

        .node {
            /* --el-tree-node-content-height: fit-content; */
            --el-tree-node-hover-bg-color: transparent;
        }

        .el-tree-node__content {
            display: flex;
            align-items: stretch;
            margin-bottom: 2px !important;
            padding-left: 0 !important;
            border: 1px solid transparent;

            &:last-child{
                margin-bottom: 0px;
            }

            &:hover{
                background: none;
                border: 1px solid var(--ks-border-active);
            }
        }

        .is-expanded {
            .el-tree-node__children {
                margin-left: 11px !important;
                padding-left: 0 !important;
                border-left: 1px solid var(--ks-border-primary);

Review comment (suggestion):
When you decide code should not be used, please remove it.
Outdated comments tend to create visual noise. They make relevant code harder to find.

```suggestion
```

Code after:
    ul.tabs-context {
        position: fixed;
        z-index: 9999;
        border: 1px solid var(--ks-border-primary);

        & li {
            height: 30px;
            padding: 16px;
            font-size: var(--el-font-size-small);
            color: var(--ks-content-primary);

            &:hover {
                color: var(--ks-content-secondary);
            }
        }
    }

    :deep(.el-tree) {
        height: calc(100% - 64px);
        overflow: auto;
        background: var(--ks-background-panel);

        .el-tree__empty-block {
            height: auto;
        }

        .node {
            --el-tree-node-hover-bg-color: transparent;
        }

        .el-tree-node__content {
            display: flex;
            align-items: center;
            margin-bottom: 2px !important;
            padding-left: 0 !important;
            border: 1px solid transparent;

            &:last-child{
                margin-bottom: 0px;
            }

            &:hover{
                background: none;
                border: 1px solid var(--ks-border-active);
            }
        }

        .is-expanded {
            .el-tree-node__children {
                margin-left: 11px !important;
                padding-left: 0 !important;
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/models/flows/FlowWithSource.java (Java)

Code before:
package io.kestra.core.models.flows;

import io.micronaut.core.annotation.Introspected;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.ToString;
import lombok.experimental.SuperBuilder;
import io.swagger.v3.oas.annotations.media.Schema;

import java.time.Instant;

@SuperBuilder(toBuilder = true)
@Getter
@NoArgsConstructor
@Introspected
@ToString
public class FlowWithSource extends Flow {

    String source;
    
    Instant updatedDate;

    @SuppressWarnings("deprecation")
    public Flow toFlow() {
        return Flow.builder()
            .tenantId(this.tenantId)
            .id(this.id)
            .namespace(this.namespace)
            .revision(this.revision)
            .description(this.description)
            .labels(this.labels)
            .inputs(this.inputs)
            .outputs(this.outputs)
            .variables(this.variables)
            .tasks(this.tasks)
            .errors(this.errors)
            ._finally(this._finally)
            .listeners(this.listeners)
            .afterExecution(this.afterExecution)
            .triggers(this.triggers)
            .pluginDefaults(this.pluginDefaults)
            .disabled(this.disabled)
            .deleted(this.deleted)
            .concurrency(this.concurrency)
            .retry(this.retry)
            .sla(this.sla)

Review comment (refactor):
Put the updatedDate inside the `AbstractFlow.java` please, as its there that we have the revision prop at first

Code after:
package io.kestra.core.models.flows;

import io.micronaut.core.annotation.Introspected;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.ToString;
import lombok.experimental.SuperBuilder;
import io.swagger.v3.oas.annotations.media.Schema;

@SuperBuilder(toBuilder = true)
@Getter
@NoArgsConstructor
@Introspected
@ToString
public class FlowWithSource extends Flow {

    String source;

    @SuppressWarnings("deprecation")
    public Flow toFlow() {
        return Flow.builder()
            .tenantId(this.tenantId)
            .id(this.id)
            .namespace(this.namespace)
            .revision(this.revision)
            .description(this.description)
            .labels(this.labels)
            .inputs(this.inputs)
            .outputs(this.outputs)
            .variables(this.variables)
            .tasks(this.tasks)
            .errors(this.errors)
            ._finally(this._finally)
            .listeners(this.listeners)
            .afterExecution(this.afterExecution)
            .triggers(this.triggers)
            .pluginDefaults(this.pluginDefaults)
            .disabled(this.disabled)
            .deleted(this.deleted)
            .concurrency(this.concurrency)
            .retry(this.retry)
            .sla(this.sla)
            .checks(this.checks)
            .build();
    }
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/models/flows/AbstractFlow.java (Java)

Code before:
import lombok.experimental.SuperBuilder;

import java.util.List;
import java.util.Map;

@SuperBuilder(toBuilder = true)
@Getter
@NoArgsConstructor
@JsonDeserialize
public abstract class AbstractFlow implements FlowInterface {
    @NotNull
    @NotBlank
    @Pattern(regexp = "^[a-zA-Z0-9][a-zA-Z0-9._-]*")
    @Size(min = 1, max = 100)
    String id;

    @NotNull
    @Pattern(regexp = "^[a-z0-9][a-z0-9._-]*")
    @Size(min = 1, max = 150)
    String namespace;

    @Min(value = 1)
    Integer revision;

    @Schema(description = "The timestamp when this revision was created or last updated.")
    java.time.Instant updatedDate;

    String description;

    @Valid
    List<Input<?>> inputs;

    @Valid
    List<Output> outputs;

    @NotNull
    @Builder.Default
    boolean disabled = false;

    @Getter
    @NotNull
    @Builder.Default
    boolean deleted = false;

    @Hidden
    @Pattern(regexp = "^[a-z0-9][a-z0-9_-]*")
    String tenantId;

    @JsonSerialize(using = ListOrMapOfLabelSerializer.class)
    @JsonDeserialize(using = ListOrMapOfLabelDeserializer.class)
    @Schema(

Review comment (suggestion):
```suggestion
    java.time.Instant updated;
```

Code after:
import lombok.experimental.SuperBuilder;

import java.time.Instant;
import java.util.List;
import java.util.Map;

@SuperBuilder(toBuilder = true)
@Getter
@NoArgsConstructor
@JsonDeserialize
public abstract class AbstractFlow implements FlowInterface {
    @NotNull
    @NotBlank
    @Pattern(regexp = "^[a-zA-Z0-9][a-zA-Z0-9._-]*")
    @Size(min = 1, max = 100)
    String id;

    @NotNull
    @Pattern(regexp = "^[a-z0-9][a-z0-9._-]*")
    @Size(min = 1, max = 150)
    String namespace;

    @Min(value = 1)
    Integer revision;

    @Schema(description = "The timestamp when this revision was created or last updated.")
    Instant updated;

    String description;

    @Valid
    List<Input<?>> inputs;

    @Valid
    List<Output> outputs;

    @NotNull
    @Builder.Default
    boolean disabled = false;

    @Getter
    @NotNull
    @Builder.Default
    boolean deleted = false;

    @Hidden
    @Pattern(regexp = "^[a-z0-9][a-z0-9_-]*")
    String tenantId;

    @JsonSerialize(using = ListOrMapOfLabelSerializer.class)
    @JsonDeserialize(using = ListOrMapOfLabelDeserializer.class)
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/models/flows/FlowWithSource.java (Java)

Code before:
        return FlowWithSource.builder()
            .tenantId(flow.tenantId)
            .id(flow.id)
            .namespace(flow.namespace)
            .revision(flow.revision)
            .description(flow.description)
            .labels(flow.labels)
            .inputs(flow.inputs)
            .outputs(flow.outputs)
            .variables(flow.variables)
            .tasks(flow.tasks)
            .errors(flow.errors)
            ._finally(flow._finally)
            .afterExecution(flow.afterExecution)
            .listeners(flow.listeners)
            .triggers(flow.triggers)
            .pluginDefaults(flow.pluginDefaults)
            .disabled(flow.disabled)
            .deleted(flow.deleted)
            .source(source)
            .concurrency(flow.concurrency)
            .retry(flow.retry)
            .sla(flow.sla)
            .checks(flow.checks)
            .build();
    }
    
    public static FlowWithSource of(Flow flow, String source, Instant updatedDate) {
        return FlowWithSource.builder()
            .tenantId(flow.tenantId)
            .id(flow.id)
            .namespace(flow.namespace)
            .revision(flow.revision)
            .description(flow.description)
            .labels(flow.labels)
            .inputs(flow.inputs)
            .outputs(flow.outputs)
            .variables(flow.variables)
            .tasks(flow.tasks)
            .errors(flow.errors)
            ._finally(flow._finally)
            .afterExecution(flow.afterExecution)
            .listeners(flow.listeners)
            .triggers(flow.triggers)
            .pluginDefaults(flow.pluginDefaults)
            .disabled(flow.disabled)
            .deleted(flow.deleted)
            .source(source)
            .updatedDate(updatedDate)
            .concurrency(flow.concurrency)
            .retry(flow.retry)

Review comment (refactor):
Property updated is in flow as it comes from `AbstractFlow` so no need for a new method
```suggestion
            .checks(flow.checks)
            .updated(flow.updated)
            .build();
    }
    
}
```

Code after:
            .id(flow.id)
            .namespace(flow.namespace)
            .revision(flow.revision)
            .description(flow.description)
            .labels(flow.labels)
            .inputs(flow.inputs)
            .outputs(flow.outputs)
            .variables(flow.variables)
            .tasks(flow.tasks)
            .errors(flow.errors)
            ._finally(flow._finally)
            .afterExecution(flow.afterExecution)
            .listeners(flow.listeners)
            .triggers(flow.triggers)
            .pluginDefaults(flow.pluginDefaults)
            .disabled(flow.disabled)
            .deleted(flow.deleted)
            .source(source)
            .concurrency(flow.concurrency)
            .retry(flow.retry)
            .sla(flow.sla)
            .checks(flow.checks)
            .updated(flow.updated)
            .build();
    }
}
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/runners/RunContextSerializer.java (Java)

Code before:
import java.util.Optional;

/**
 * Custom serializer for {@link RunContext}.
 * <p>
 * Filters out the "envs" key from the variables map during serialization ensuring that environment
 * variables are not sent to workers.
 */
public class RunContextSerializer extends StdSerializer<RunContext> {

    @Serial
    private static final long serialVersionUID = 1L;

    protected RunContextSerializer() {
        super(RunContext.class);
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void serialize(RunContext value, JsonGenerator gen, SerializerProvider provider) throws IOException {

        Map<String, Object> contextMap = new HashMap<>(3);
        Optional.ofNullable(value.getSecretInputs()).ifPresent(items -> contextMap.put("secretInputs", items));
        Optional.ofNullable(value.getTraceParent()).ifPresent(items -> contextMap.put("traceParent", items));

        Map<String, Object> mutableVariables = new HashMap<>(value.getVariables());
        mutableVariables.remove("envs");
        contextMap.put("variables", mutableVariables);

        gen.writeObject(contextMap);
    }
}


Review comment (suggestion):
This is a little concerning as if we add more base attributes to the runContext at some point, we would need to add them here also.
I wonder if we should not copy all variables and only remove the envs.

Otherwize, a comment inside the runContext that point here would be mandatory so we change the two places.

Code after:
import java.util.List;
import java.util.Map;

/**
 * Custom serializer for {@link RunContext}.
 * <p>
 * Filters out the "envs" key from the variables map during serialization ensuring that environment
 * variables are not sent to workers.
 */
public class RunContextSerializer extends StdSerializer<RunContext> {

    @Serial
    private static final long serialVersionUID = 1L;

    protected RunContextSerializer() {
        super(RunContext.class);
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void serialize(RunContext value, JsonGenerator gen, SerializerProvider provider) throws IOException {
        Map<String, Object> mutableVariables = new HashMap<>(value.getVariables());
        mutableVariables.remove("envs");
        gen.writeObject(new RunContextData(mutableVariables, value.getSecretInputs(), value.getTraceParent()));
    }

    /**
     * Intermediate record used to preserve null values during serialization.
     * <p>
     * The {@code @JsonInclude} annotation (defaulting to {@code ALWAYS} for both value and content)
     * overrides the mapper-level {@code NON_NULL} policy, matching the behavior of {@link RunContext}'s property annotations.
     */
    private record RunContextData(
        @JsonInclude
        Map<String, Object> variables,
        @JsonInclude
        List<String> secretInputs,
        @JsonInclude
        String traceParent
    ) {}
}