feat(core): add required field to prevent unconfigured steps from running by timothysparg · Pull Request #785 · jdx/hk

timothysparg · 2026-03-29T22:48:35Z

Some linters require user-provided configuration to be useful. I ran into this when I wanted to add addLicense as a builtin linter.

@Builtins.meta {
  category = "Special Purpose"
  description = "Manage license headers"
}

addlicense = new Config.Step {
  batch = true
  check = "addlicense -check {{ files }}"
  fix = "addlicense  {{ files }}"
}

With this configuration addLicense will default to an Apache license, with Google LLC as the copyright holder. Which for the vast majority of people would be a misconfiguration.

If we add this as a Builtin then in the hk.pkl users would end up with something like

  ["addlicense"] = (Builtins.addlicense) {
    check = "addlicense --check -f MY_LICENSE {{files}}"
    fix = "addlicense -f MY_LICENSE {{files}}"
  }

Which doesn't really feel like much of a win.

I came across step_condition which allowed me to do the following

step_condition = "exec('if [ -n \"$LICENSE_FILE\" ] && [ -f \"$LICENSE_FILE\" ]; then echo -n 1; fi') == '1'"
addlicense = new Config.Step {
  batch = true
  check = "addlicense -check {{ files }}"
  fix = "addlicense  {{ files }}"
}

but I'm not convinced that this is really what step_condition was designed for. I think it could work in a pinch, but that syntax is a but meh from a usability perspective.

So it seems that we are currently have the following options

Run with a wrong default
Force users to override the entire check/fix command (defeats the purpose of a builtin)
Use step_condition with a shell expression, which feels clunky

Proposal

Add a required field to Step that declares which environment variables must be set for the step
to run. If any are missing, the step is gracefully skipped with a clear message rather than
running incorrectly.

["addlicense"] {
required = List("LICENSE_FILE")
check = "addlicense --check -f $LICENSE_FILE {{files}}"
fix = "addlicense -f $LICENSE_FILE {{files}}"
}

A variable is considered satisfied if it is present in any of:

The process environment
The global env block in hk.pkl
The step's own env block

The skip message is surfaced via display_skip_reasons:
display_skip_reasons = List("missing-required-env")

gemini-code-assist

Code Review

This pull request introduces a required field for steps, allowing users to specify environment variables that must be present for a step to execute. The implementation includes updates to the Pkl configuration, Rust logic for environment variable validation, and new integration tests. The feedback suggests adding a test case to ensure that environment variables defined within the configuration file itself (the env block) correctly satisfy the requirement.

gemini-code-assist · 2026-03-29T22:49:36Z

+@test "required: step is skipped when env is missing" {
+  cat >hk.pkl <<EOF
+amends "$PKL_PATH/Config.pkl"
+
+display_skip_reasons = List("missing-required-env")
+
+hooks = new {
+    ["check"] {
+        steps = new {
+            ["test-required"] {
+                required = List("MANDATORY_VAR")
+                check = "echo 'Success'"
+            }
+        }
+    }
+}
+EOF
+
+  run hk check --all
+  assert_success
+  assert_output --partial "skipped: missing required environment variable(s): MANDATORY_VAR"
+}
+
+@test "required: step runs when env is provided" {
+  cat >hk.pkl <<EOF
+amends "$PKL_PATH/Config.pkl"
+
+hooks = new {
+    ["check"] {
+        steps = new {
+            ["test-required"] {
+                required = List("MANDATORY_VAR")
+                check = "echo 'Success'"
+            }
+        }
+    }
+}
+EOF
+
+  MANDATORY_VAR=1 run hk check --all
+  assert_success
+  assert_output --partial "test-required"
+  refute_output --partial "skipped"
+}
+
+@test "required: skip message can be hidden" {
+  cat >hk.pkl <<EOF
+amends "$PKL_PATH/Config.pkl"
+
+display_skip_reasons = List() // Hide all
+
+hooks = new {
+    ["check"] {
+        steps = new {
+            ["test-required"] {
+                required = List("MANDATORY_VAR")
+                check = "echo 'Success'"
+            }
+        }
+    }
+}
+EOF
+
+  run hk check --all
+  assert_success
+  refute_output --partial "skipped: missing required environment variable(s)"
+}


The new tests cover cases where the environment variable is missing or provided via the process environment. It would be beneficial to also add a test case where the required environment variable is defined within hk.pkl itself, for example in the step's env block. This would ensure all methods of satisfying the requirement are covered by tests.

Here's a suggestion for such a test:

@test "required: step runs when env is provided in hk.pkl" { cat >hk.pkl <<EOF amends "$PKL_PATH/Config.pkl" hooks = new { ["check"] { steps = new { ["test-required"] { required = List("MANDATORY_VAR") check = "echo 'Success'" env = new Mapping { ["MANDATORY_VAR"] = "1" } } } } } EOF run hk check --all assert_success assert_output --partial "test-required" refute_output --partial "skipped" }

…ning

greptile-apps · 2026-03-29T23:27:17Z

Greptile Summary

This PR introduces a required field on Step that lists environment variable names which must be present for a step to execute. When any required variable is absent, build_step_jobs returns a single skipped job carrying the new MissingRequiredEnv skip reason, and the message is surfaced through the existing display_skip_reasons mechanism. The feature is motivated by built-in linters (e.g. addlicense) that need user-supplied configuration to behave correctly.

Key changes:

pkl/Config.pkl: Adds required: List<String> = List() to Step and documents \"missing-required-env\" as a valid display_skip_reasons value.
src/step/types.rs: Adds required: Vec<String> with #[serde(default)] to the Rust Step struct.
src/hook.rs: Adds MissingRequiredEnv(Vec<String>) to SkipReason with a human-readable message() and correct strum Display integration for should_display() matching.
src/step/job_builder.rs: Inserts the required-env check between the "no command" check and file filtering. Satisfaction is determined by std::env::var (covers process env and global env block, which config.init() pushes into the process env via std::env::set_var) and self.env.contains_key (covers step-level and hook-level env, the latter merged in by hook.init()).
test/required_env.bats: Three tests covering skip-on-missing, run-when-present-in-process-env, and run-when-present-in-step's-own-env block.

One prior review concern remains open: std::env::var(e).is_err() in job_builder.rs incorrectly skips a step when the named variable is set but contains non-Unicode bytes (VarError::NotUnicode); std::env::var_os(e).is_none() would handle this correctly. Test coverage for the global and hook-level env block paths is also still absent.

Confidence Score: 4/5

Safe to merge for the common case; one pre-existing P1 from a prior review thread (var().is_err() non-Unicode false-positive) is still unresolved.

No new P0/P1 issues were found in this review pass. The sole new finding is a redundant strum annotation (P2). However, the std::env::var(e).is_err() defect flagged in the prior review — which causes a step to be incorrectly skipped when a required variable exists but holds non-Unicode bytes — is still present in the diff, keeping the score at 4 rather than 5.

src/step/job_builder.rs — the var().is_err() check on line 72 should be var_os().is_none() to avoid false-positive skips on non-Unicode env var values.

Important Files Changed

Filename	Overview
src/step/job_builder.rs	Adds the `required` env var check before file filtering; uses `std::env::var().is_err()` which incorrectly skips steps for non-unicode var values (flagged in prior review, still unresolved).
src/hook.rs	Adds `MissingRequiredEnv(Vec<String>)` SkipReason variant with message formatting and strum Display integration; logic is sound.
src/step/types.rs	Adds `required: Vec<String>` field with `#[serde(default)]`; correctly placed and annotated.
pkl/Config.pkl	Adds `required` field and `"missing-required-env"` skip reason doc; doc comment omits hook-level env as a satisfaction source (flagged in prior review).
test/required_env.bats	Three bats tests covering skip-on-missing, run-on-present, and step-level env block; missing coverage for global env and hook-level env blocks (flagged in prior review).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build_step_jobs called] --> B{Step in skip_steps?}
    B -- Yes --> C[Return job with existing SkipReason]
    B -- No --> D{has_command_for run_type?}
    D -- No --> E[Return job with NoCommandForRunType]
    D -- Yes --> F{required is non-empty?}
    F -- No --> I[filter_files]
    F -- Yes --> G["Check each var:\nstd::env::var(e).is_err()\n&& !self.env.contains_key(e)"]
    G --> H{Any missing?}
    H -- Yes --> J[Return job with MissingRequiredEnv]
    H -- No --> I
    I --> K{files empty\n& has_filters?}
    K -- Yes --> L[Return job with NoFilesToProcess]
    K -- No --> M[Create workspace/batch jobs]

    subgraph "env satisfaction sources"
        N[process env\nincl. global env block\nvia std::env::set_var]
        O[step.env\nincl. hook-level env\nmerged by hook.init]
    end

    G -.->|std::env::var| N
    G -.->|self.env.contains_key| O

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into feat/core/requi..." | Re-trigger Greptile}

greptile-apps · 2026-03-29T23:27:21Z

+            let missing: Vec<String> = self
+                .required
+                .iter()
+                .filter(|e| std::env::var(e).is_err() && !self.env.contains_key(*e))


Use var_os().is_none() instead of var().is_err() to detect unset variables

std::env::var(e).is_err() returns true for two distinct cases:

The variable is not set (VarError::NotPresent) — correct, should skip.

The variable is set but its value contains non-Unicode bytes (VarError::NotUnicode) — incorrect, step should run.

Using std::env::var_os(e).is_none() correctly checks only for the "not set" case, since var_os accepts non-Unicode bytes and returns Some(OsString) whenever the key exists.

Suggested change

.filter(|e| std::env::var(e).is_err() && !self.env.contains_key(*e))

.filter(|e| std::env::var_os(e).is_none() && !self.env.contains_key(*e))

greptile-apps · 2026-03-29T23:27:22Z

 class Step {
+  /// List of environment variables that must be set for this step to run.
+  /// A variable is considered satisfied if it is present in the process environment,
+  /// the global `env` block in hk.pkl, or the step's own `env` block.
+  /// If any are missing, the step will be skipped with a clear message.


Doc omits hook-level env block as a valid source

At runtime, hook.init() merges the hook-level env map into every step's env map before build_step_jobs is called (see src/hook.rs lines 304–319). As a result, variables declared in the hook's env block also satisfy the required check via self.env.contains_key(e), but the doc comment only mentions "the step's own env block".

Consider updating the doc to reflect all sources that satisfy the check:

Suggested change

class Step {

/// List of environment variables that must be set for this step to run.

/// A variable is considered satisfied if it is present in the process environment,

/// the global `env` block in hk.pkl, or the step's own `env` block.

/// If any are missing, the step will be skipped with a clear message.

/// List of environment variables that must be set for this step to run.

/// A variable is considered satisfied if it is present in the process environment,

/// the global `env` block in hk.pkl, the hook's `env` block,

/// or the step's own `env` block.

/// If any are missing, the step will be skipped with a clear message.

required: List<String> = List()

greptile-apps · 2026-03-29T23:27:23Z

+  assert_output --partial "test-required"
+  refute_output --partial "skipped"
+}


Missing test coverage for global env block and hook-level env block

The PR description and the Config.pkl doc both state that a required variable is satisfied when it appears in the global env block of hk.pkl. This path is exercised via std::env::set_var in config.init(), but there is no bats test confirming it. Likewise, the hook-level env block (merged into step.env during hook.init()) is also an accepted source but has no test.

Consider adding tests similar to the third test but using:

A top-level env { ["MANDATORY_VAR"] = "1" } block in the config (global env).

An env { ["MANDATORY_VAR"] = "1" } block on the hook rather than the step (hook env).

This would guard against regressions in the merging logic that makes these sources work.

@hituzi-no-sippo

### 🚀 Features - **(betterleaks)** add betterleaks config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#750](#750) - **(builtins)** add google-java-format to builtins by [@timothysparg](https://github.com/timothysparg) in [#777](#777) - **(builtins)** add dclint to builtins by [@timothysparg](https://github.com/timothysparg) in [#779](#779) - **(config)** set default value for exclude to List() by [@timothysparg](https://github.com/timothysparg) in [#781](#781) - **(core)** add required field to prevent unconfigured steps from running by [@timothysparg](https://github.com/timothysparg) in [#785](#785) - **(gitleaks)** add gitleaks config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#749](#749) - **(mdschema)** add mdschema config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#748](#748) - **(pkl)** add pklr as opt-in pkl backend by [@jdx](https://github.com/jdx) in [#769](#769) - add pklr as opt-in pkl backend by [@jdx](https://github.com/jdx) in [#768](#768) ### 🐛 Bug Fixes - **(docs)** replace invalid /latest/ pkl package URIs with versioned format by [@jdx](https://github.com/jdx) in [#770](#770) - **(stage)** do not stage pre-existing untracked files by [@jdx](https://github.com/jdx) in [#788](#788) ### 📚 Documentation - add benchmarks page and reproducible benchmark suite by [@jdx](https://github.com/jdx) in [#766](#766) - add recommended setup section to mise integration by [@timothysparg](https://github.com/timothysparg) in [#780](#780) ### 📦️ Dependency Updates - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#762](#762) - update rust crate pklr to 0.4 by [@renovate[bot]](https://github.com/renovate[bot]) in [#776](#776) - update apple-actions/import-codesign-certs digest to fe74d46 by [@renovate[bot]](https://github.com/renovate[bot]) in [#774](#774) - update anthropics/claude-code-action digest to 094bd24 by [@renovate[bot]](https://github.com/renovate[bot]) in [#773](#773) - update taiki-e/upload-rust-binary-action digest to 0e34102 by [@renovate[bot]](https://github.com/renovate[bot]) in [#775](#775) - bump usage to 3.2.0 and pkl to 0.31.1, add windows platforms by [@jdx](https://github.com/jdx) in [#787](#787) - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#786](#786) ### New Contributors - @timothysparg made their first contribution in [#781](#781)  --- > [!NOTE] > **Medium Risk** > Primarily a version/documentation bump, but it also updates the Rust dependency lockfile (e.g., `hyper` and `windows-sys`), which could introduce build/runtime regressions. > > **Overview** > Bumps hk to **v1.40.0** and publishes the corresponding release notes in `CHANGELOG.md`. > > Updates generated CLI/docs and all Pkl package URL references in docs/examples to point at `v1.40.0`, and refreshes `Cargo.lock` with dependency updates/removals consistent with the new release. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit da00ab8. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>  Co-authored-by: mise-en-dev <123107610+mise-en-dev@users.noreply.github.com>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [hk](https://github.com/jdx/hk) | minor | `1.39.0` → `1.40.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jdx/hk (hk)</summary> ### [`v1.40.0`](https://github.com/jdx/hk/blob/HEAD/CHANGELOG.md#1400---2026-04-01) [Compare Source](jdx/hk@v1.39.0...v1.40.0) ##### 🚀 Features - **(betterleaks)** add betterleaks config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#750](jdx/hk#750) - **(builtins)** add google-java-format to builtins by [@timothysparg](https://github.com/timothysparg) in [#777](jdx/hk#777) - **(builtins)** add dclint to builtins by [@timothysparg](https://github.com/timothysparg) in [#779](jdx/hk#779) - **(config)** set default value for exclude to List() by [@timothysparg](https://github.com/timothysparg) in [#781](jdx/hk#781) - **(core)** add required field to prevent unconfigured steps from running by [@timothysparg](https://github.com/timothysparg) in [#785](jdx/hk#785) - **(gitleaks)** add gitleaks config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#749](jdx/hk#749) - **(mdschema)** add mdschema config to hk builtin config by [@hituzi-no-sippo](https://github.com/hituzi-no-sippo) in [#748](jdx/hk#748) - **(pkl)** add pklr as opt-in pkl backend by [@jdx](https://github.com/jdx) in [#769](jdx/hk#769) - add pklr as opt-in pkl backend by [@jdx](https://github.com/jdx) in [#768](jdx/hk#768) ##### 🐛 Bug Fixes - **(docs)** replace invalid /latest/ pkl package URIs with versioned format by [@jdx](https://github.com/jdx) in [#770](jdx/hk#770) - **(stage)** do not stage pre-existing untracked files by [@jdx](https://github.com/jdx) in [#788](jdx/hk#788) ##### 📚 Documentation - add benchmarks page and reproducible benchmark suite by [@jdx](https://github.com/jdx) in [#766](jdx/hk#766) - add recommended setup section to mise integration by [@timothysparg](https://github.com/timothysparg) in [#780](jdx/hk#780) ##### 📦️ Dependency Updates - lock file maintenance by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#762](jdx/hk#762) - update rust crate pklr to 0.4 by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#776](jdx/hk#776) - update apple-actions/import-codesign-certs digest to [`fe74d46`](jdx/hk@fe74d46) by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#774](jdx/hk#774) - update anthropics/claude-code-action digest to [`094bd24`](jdx/hk@094bd24) by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#773](jdx/hk#773) - update taiki-e/upload-rust-binary-action digest to [`0e34102`](jdx/hk@0e34102) by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#775](jdx/hk#775) - bump usage to 3.2.0 and pkl to 0.31.1, add windows platforms by [@jdx](https://github.com/jdx) in [#787](jdx/hk#787) - lock file maintenance by [@renovate\[bot\]](https://github.com/renovate\[bot]) in [#786](jdx/hk#786) ##### New Contributors - [@timothysparg](https://github.com/timothysparg) made their first contribution in [#781](jdx/hk#781) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

gemini-code-assist Bot reviewed Mar 29, 2026

View reviewed changes

feat(core): add required field to prevent unconfigured steps from run…

e186134

…ning

timothysparg force-pushed the feat/core/required-field branch from 0980022 to e186134 Compare March 29, 2026 23:12

timothysparg marked this pull request as ready for review March 29, 2026 23:23

greptile-apps Bot reviewed Mar 29, 2026

View reviewed changes

Merge branch 'main' into feat/core/required-field

16637ce

jdx merged commit 5f678e7 into jdx:main Apr 1, 2026
18 checks passed

jdx mentioned this pull request Apr 1, 2026

chore: release v1.40.0 #763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(core): add required field to prevent unconfigured steps from running#785

feat(core): add required field to prevent unconfigured steps from running#785
jdx merged 2 commits intojdx:mainfrom
timothysparg:feat/core/required-field

timothysparg commented Mar 29, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Mar 29, 2026

Uh oh!

greptile-apps Bot commented Mar 29, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Mar 29, 2026

Uh oh!

greptile-apps Bot Mar 29, 2026

Uh oh!

greptile-apps Bot Mar 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	.filter(\|e\| std::env::var(e).is_err() && !self.env.contains_key(*e))
	.filter(\|e\| std::env::var_os(e).is_none() && !self.env.contains_key(*e))

Uh oh!

Conversation

timothysparg commented Mar 29, 2026

Proposal

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented Mar 29, 2026 •

edited

Loading