Conversation
hkrc files that amend Config.pkl (as shown in the docs) panic because
hk deserializes them as UserConfig where check is Option<bool>, not a
string command. Also covers missing home directory default path, hook
merging, and step precedence.
Current error:
thread 'main' panicked at src/settings.rs:90:30:
Failed to load configuration: failed to run pkl
Caused by:
0: failed to parse pkl output
1: invalid type: string "echo 'eslint check'",
expected a boolean at line 21 column 40
Location:
src/config.rs:335:37
hkrc files that amend Config.pkl panicked because they were always deserialized as UserConfig (where check is Option<bool>). Now hkrc loading detects the format via JSON keys and dispatches accordingly. Also fixes the default hkrc path: discovers ~/.hkrc.pkl instead of only looking at CWD-relative .hkrc.pkl.
Add ~/.config/hk/config.pkl as a third discovery path (after CWD and ~/.hkrc.pkl) following XDG conventions already used for state and cache dirs. Fix docs: correct precedence semantics to "project wins", replace UserConfig.pkl example with Config.pkl, document the full discovery order, and note legacy UserConfig.pkl support.
Replace if-is_none blocks with idiomatic Option::or in merge_from_hkrc, annotate backward-compat format detection and the unsafe set_var, use latest pkg URL in docs example, and mention hk.local.pkl in merge semantics.
No tests covered hk.local.pkl winning over hk.pkl, or hkrc discovery order when multiple candidates exist. These three tests pin the full hierarchy: local > project, CWD hkrc > home hkrc > XDG config.
The settings table had hkrc (5) beating project config (6) but the actual merge does project-wins. Swap rows 5-6 to match the code, and add a top-level summary table so readers see the full layering without hunting across three sections.
Users who want to enforce their own hooks, skip project steps, or fully replace a project's config locally had no doc guidance — just mechanics. Add three concrete patterns for the "grumpy staff eng" use case.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical bugs and ambiguities in Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, addressing bugs in the hkrc configuration handling and significantly clarifying the documentation around configuration loading and precedence. The introduction of a unified hkrc format that supports full Config.pkl features is a major step forward, and the backward compatibility for legacy UserConfig.pkl files is well-handled. The new test cases are comprehensive and provide good coverage for the new logic.
I've found one high-severity issue where the environment variable merging logic contradicts the new "project wins" precedence documented in this PR. I've also left a couple of medium-severity suggestions in src/config.rs to refactor parts of the new logic for better conciseness and maintainability.
Overall, excellent work on untangling the configuration logic and improving the user experience.
| // Scalar settings: project wins — fall back to hkrc when project has None | ||
| self.fail_fast = self.fail_fast.or(hkrc.fail_fast); | ||
| self.stage = self.stage.or(hkrc.stage); | ||
| self.display_skip_reasons = self.display_skip_reasons.take().or(hkrc.display_skip_reasons); | ||
| self.hide_warnings = self.hide_warnings.take().or(hkrc.hide_warnings); | ||
| self.warnings = self.warnings.take().or(hkrc.warnings); | ||
| self.exclude = self.exclude.take().or(hkrc.exclude); | ||
| self.profiles = self.profiles.take().or(hkrc.profiles); | ||
| self.skip_hooks = self.skip_hooks.take().or(hkrc.skip_hooks); | ||
| self.skip_steps = self.skip_steps.take().or(hkrc.skip_steps); | ||
| self.default_branch = self.default_branch.take().or(hkrc.default_branch); | ||
| self.min_hk_version = self.min_hk_version.take().or(hkrc.min_hk_version); |
There was a problem hiding this comment.
This block of code for merging scalar settings is quite repetitive. To improve maintainability and reduce boilerplate, consider using a macro to handle the merging pattern, especially for the non-Copy types that use .take().
There was a problem hiding this comment.
First pass was a big block of if-statements. 😓 I'm not sure macros are gonna do anything other than make the logic more difficult to understand. But, I'm not a Rust expert.
| | Precedence | Source | Scope | | ||
| |---|---|---| | ||
| | 1 (lowest) | Built-in defaults | All projects | | ||
| | 2 | [hkrc](#hkrc) (`~/.config/hk/config.pkl`, `~/.hkrc.pkl`, or `--hkrc`) | All projects (user-level) | | ||
| | 3 | [Project config](#hk-pkl) (`hk.pkl` or `hk.local.pkl`) | Single project | | ||
| | 4 | [Git config](#git-configuration) (global, then local) | Per-repo | | ||
| | 5 | [Environment variables](#settings-reference) (`HK_*`) | Per-invocation | | ||
| | 6 (highest) | [CLI flags](#settings-reference) | Per-invocation | |
There was a problem hiding this comment.
This is mise…ish. Thoughts?
| ```bash | ||
| # Skip a step permanently in this repo | ||
| git config --local hk.skipSteps "slow-linter,noisy-formatter" | ||
|
|
||
| # Skip for one run | ||
| HK_SKIP_STEPS=slow-linter hk run pre-commit | ||
| ``` |
There was a problem hiding this comment.
I thought it'd be a helpful example for grumbling engineers who run into hooks that aren't their style.
| **Completely replace a project's hooks locally** | ||
|
|
||
| Create `hk.local.pkl` in the project root (don't commit it). It replaces `hk.pkl` entirely — redefine only what you want: |
There was a problem hiding this comment.
Nuclear option. This was already the existing behavior but there wasn't a test covering this (so I added one)!
| > [!NOTE] | ||
| > Legacy hkrc files that amend `UserConfig.pkl` are still supported. | ||
|
|
There was a problem hiding this comment.
Not sure how/if you want to maintain backwards compatibility.
| assert_output --partial "hello" | ||
| } | ||
|
|
||
| @test "hk.local.pkl takes precedence over hk.pkl" { |
There was a problem hiding this comment.
Explicit test for hk.local.pkl > hk.pkl (mentioned in PR body)
Address PR feedback: env vars from hkrc now only set when the project config hasn't defined them (matching the documented precedence). Also simplify hkrc path discovery with array + find().
Greptile SummaryFixes two critical bugs in hkrc handling and establishes clear config precedence semantics: Fixed Issues:
Key Changes:
Note: Legacy UserConfig format maintains "user wins" behavior for backward compatibility, while new Config format uses "project wins" as documented. Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[hk starts] --> B[Load project config<br/>hk.pkl or hk.local.pkl]
B --> C[apply_hkrc called]
C --> D{--hkrc flag set?}
D -->|Yes| E[Use explicit path<br/>Show deprecation warning]
D -->|No| F[Discovery mode]
F --> G{.hkrc.pkl in CWD?}
G -->|Yes| H[Use CWD .hkrc.pkl<br/>Show deprecation]
G -->|No| I{~/.hkrc.pkl exists?}
I -->|Yes| J[Use HOME .hkrc.pkl<br/>Show deprecation]
I -->|No| K{XDG config.pkl exists?}
K -->|Yes| L[Use XDG config.pkl<br/>No warning]
K -->|No| M[No hkrc]
E --> N{Parse pkl as JSON}
H --> N
J --> N
L --> N
N --> O{Has 'environment' key?}
O -->|Yes| P[Parse as UserConfig<br/>Legacy format]
O -->|No| Q[Parse as Config<br/>New format]
P --> R[apply_user_config<br/>User wins for env]
Q --> S[merge_from_hkrc<br/>Project wins for env]
R --> T[Validate config]
S --> T
M --> T
T --> U[Ready to run]
Last reviewed commit: 5082fae |
| if let Some(project_hook) = self.hooks.get_mut(&hook_name) { | ||
| for (step_name, hkrc_step) in hkrc_hook.steps { | ||
| project_hook.steps.entry(step_name).or_insert(hkrc_step); | ||
| } |
There was a problem hiding this comment.
Consider merging hook-level properties (fix, stash, stage, env) from hkrc when hooks collide. Currently only steps merge — hkrc hook properties are lost.
if let Some(project_hook) = self.hooks.get_mut(&hook_name) {
// Merge hook-level properties with project-wins semantics
project_hook.fix = project_hook.fix.or(hkrc_hook.fix);
project_hook.stash = project_hook.stash.clone().or(hkrc_hook.stash.clone());
project_hook.stage = project_hook.stage.or(hkrc_hook.stage);
// ... merge env, report similarly
for (step_name, hkrc_step) in hkrc_hook.steps {
project_hook.steps.entry(step_name).or_insert(hkrc_step);
}
}|
Honestly I wonder if we should deprecate This is the allure of Pkl, that's is fungible |
|
Yeah I think we should deprecate hkrc |
|
On it! 🫡 Should have more time for this tomorrow. Did you have a path in mind? I'll probably search around the codebase for some existing deprecation warnings (maybe check mise) and follow whatever pattern's there. Current version is v1.36.0. Deprecate and remove in v2? 🤷♀️ |
|
yeah I think deprecate and remove in v2 sounds good. In mise I use this pattern as a reminder when the version changes to remove deprecated code: |
Deprecate .hkrc.pkl (CWD and HOME) and --hkrc flag in favor of ~/.config/hk/config.pkl (global) and hk.local.pkl (per-project). Adopts mise's deprecated_at! pattern with version-gated warnings and debug_assert! to remind developers to remove in v2. Nothing is removed — all hkrc code paths continue to work.
|
The plan's here: https://gist.github.com/ivy/8cd019536936cabe89ce20e2f7c8aac4 Will add some comments in a sec about some implementation decisions I made. |
ivy
left a comment
There was a problem hiding this comment.
Verified locally with a throwaway test locally. Total overkill, but I'm not super familiar with this codebase. 😅
| pub static VERSION: LazyLock<semver::Version> = LazyLock::new(|| { | ||
| env!("CARGO_PKG_VERSION") | ||
| .parse() | ||
| .expect("Malformed version") | ||
| }); |
There was a problem hiding this comment.
Naming: Mise has a cli::version::V because of a conflict with VERSION (string form) but in hk, we have a version() method instead.
| repository = "https://github.com/jdx/hk" | ||
| rust-version = "1.88.0" | ||
| version = "1.36.0" | ||
| version = "1.37.0" |
There was a problem hiding this comment.
Bumped the version since 1.36.0 is already released and the tests won't work otherwise. This might conflict with the communique?
| "2.0.0", | ||
| "hkrc-home", | ||
| "~/.hkrc.pkl is deprecated. Use {}/config.pkl instead.", | ||
| env::HK_CONFIG_DIR.display() |
There was a problem hiding this comment.
I'm formatting HK_CONFIG_DIR in case it differs instead of printing the default path.
|
Test flake? On Windows: |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [hk](https://github.com/jdx/hk) | minor | `1.36.0` → `1.38.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.38.0`](https://github.com/jdx/hk/blob/HEAD/CHANGELOG.md#1380---2026-03-06) [Compare Source](jdx/hk@v1.37.0...v1.38.0) ##### 🚀 Features - **(hook)** add `fail_on_fix` option by [@​jdx](https://github.com/jdx) in [#​725](jdx/hk#725) ##### 🐛 Bug Fixes - **(builtins)** remove redundant check/check\_diff from builtins by [@​nkakouros](https://github.com/nkakouros) in [#​726](jdx/hk#726) ##### 📦️ Dependency Updates - update anthropics/claude-code-action digest to [`26ec041`](jdx/hk@26ec041) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​720](jdx/hk#720) - update jdx/mise-action digest to [`e79ddf6`](jdx/hk@e79ddf6) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​721](jdx/hk#721) - update actions-rust-lang/setup-rust-toolchain digest to [`a0b538f`](jdx/hk@a0b538f) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​719](jdx/hk#719) - update rust crate tokio to v1.50.0 by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​722](jdx/hk#722) ### [`v1.37.0`](https://github.com/jdx/hk/blob/HEAD/CHANGELOG.md#1370---2026-03-03) [Compare Source](jdx/hk@v1.36.0...v1.37.0) ##### 🚀 Features - **(hook)** add env support to hooks by [@​jdx](https://github.com/jdx) in [#​709](jdx/hk#709) - parse Go-style diffs by [@​jdx](https://github.com/jdx) in [#​704](jdx/hk#704) ##### 🐛 Bug Fixes - **(builtins)** strip extra trailing newlines in end-of-file-fixer by [@​jdx](https://github.com/jdx) in [#​708](jdx/hk#708) - **(docs)** correctly document what --all is about by [@​nkakouros](https://github.com/nkakouros) in [#​715](jdx/hk#715) - **(git)** exclude untracked files from unstaged\_files set by [@​nkakouros](https://github.com/nkakouros) in [#​716](jdx/hk#716) - **(hkrc)** config format and load order by [@​ivy](https://github.com/ivy) in [#​710](jdx/hk#710) - **(release)** write release notes to file instead of capturing stdout by [@​jdx](https://github.com/jdx) in [#​688](jdx/hk#688) - **(release)** make release notes editorialization non-blocking by [@​jdx](https://github.com/jdx) in [#​690](jdx/hk#690) - **(step)** gate check\_diff forced check\_first on Fix mode only by [@​nkakouros](https://github.com/nkakouros) in [#​717](jdx/hk#717) ##### 📚 Documentation - **(shanty)** add audio player with sea shanty recording by [@​jdx](https://github.com/jdx) in [67a25ad](jdx/hk@67a25ad) - document config file search paths by [@​ivy](https://github.com/ivy) in [#​701](jdx/hk#701) - require AI disclosure on GitHub comments by [@​jdx](https://github.com/jdx) in [#​703](jdx/hk#703) ##### 🔍 Other Changes - replace gen-release-notes script with communique by [@​jdx](https://github.com/jdx) in [#​700](jdx/hk#700) - add autofix.ci workflow by [@​jdx](https://github.com/jdx) in [#​705](jdx/hk#705) ##### 📦️ Dependency Updates - lock file maintenance by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​686](jdx/hk#686) - update taiki-e/upload-rust-binary-action digest to [`f391289`](jdx/hk@f391289) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​692](jdx/hk#692) - update anthropics/claude-code-action digest to [`c22f7c3`](jdx/hk@c22f7c3) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​691](jdx/hk#691) - update rust crate libc to v0.2.181 by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​694](jdx/hk#694) - update rust crate clap to v4.5.58 by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​693](jdx/hk#693) - lock file maintenance by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​695](jdx/hk#695) - update anthropics/claude-code-action digest to [`edd85d6`](jdx/hk@edd85d6) by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​698](jdx/hk#698) - update rust crate clap to v4.5.60 by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​699](jdx/hk#699) - lock file maintenance by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​702](jdx/hk#702) - lock file maintenance by [@​renovate\[bot\]](https://github.com/renovate\[bot]) in [#​711](jdx/hk#711) ##### New Contributors - [@​ivy](https://github.com/ivy) made their first contribution in [#​710](jdx/hk#710) - [@​nkakouros](https://github.com/nkakouros) made their first contribution in [#​715](jdx/hk#715) </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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My40OS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNTcuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
I ran into two bugs (or… features? 😉). Gory details here with a script to reproduce. Human summary:
I tried following the docs to add a global hk config and found that the documented example didn't actually work:
.hkrc.pklamends from Config.pkl (as shown in the docs). hk deserializes them asUserConfig, where check isOption<bool>, not a string command.PathBuf::from(".hkrc.pkl")(relative to CWD), not~/.hkrc.pklas the docs state.I went down a rabbit hole and added some tests to cover the bug as well as some tests to cover a clarified hierarchy:
Let me know if you hate it! 😅
Why I
wastedinvested 3 hours hacking on thisI'm trying out solutions to auto-commit and bake my LLM prompts into Git commit messages for better transparency. Whoops! I committed everything:
So I wanted to add a global pre-commit hook (with hk) that runs gitleaks from now on.