feat(check): implement --plan, --why, and --json by jdx · Pull Request #848 · jdx/hk

jdx · 2026-04-19T01:30:26Z

Summary

--plan / -P: dry-run a hook to see which steps would run, which would be skipped, and why — without executing any commands. Replaces the previous stub that panicked via todo!().
--why [STEP] / -W: show all reasons per step. Pass a step name to focus on one; omit the value to show details for every step. Implies --plan.
--json / -J: emit the plan as JSON (requires --plan) for tooling integration.

The plan is built via static analysis (reusing build_step_jobs and profile_skip_reason), so it never executes step commands. It reports step status (included/skipped), parallel-group membership, dependencies, matched file count, and a structured list of reasons (filter match, profile include/exclude, condition true/false, CLI/env/config skip, missing required env, no command for run type).

Example

$ hk check --plan
Plan: check
Run type: check

  [parallel group] group_0
    ○ actionlint  (no files matched filters)
    ✓ cargo-fmt  (6 files matched)
    ○ cargo-clippy  (required profile(s) not enabled: slow)
    ✓ cargo-check  (6 files matched)

$ hk check --plan --json | jq '.steps[0]'
{
  "name": "actionlint",
  "status": "skipped",
  "orderIndex": 0,
  "parallelGroupId": "group_0",
  "reasons": [{ "kind": "filter_no_match", "detail": "no files matched filters" }],
  "fileCount": 0
}

Test plan

13 new bats tests in test/plan.bats (26 variant runs × libgit2/nolibgit2 pass)
Covers: text+JSON output, profile gating, condition true/false, dependsOn, --step/--skip-step, the "does not execute" invariant, and --why focus modes
145 cargo test pass
No new clippy warnings
Regenerated hk.usage.kdl and docs/cli/*

🤖 Generated with Claude Code

Note

Medium Risk
Adds new CLI flags and a new plan-generation path that affects how hooks are dispatched (--plan/--why/--json) and how step conditions/skips are evaluated for reporting. While it’s largely additive, it touches core hook execution plumbing and introduces new output/validation behavior that could impact existing workflows or automation.

Overview
Implements a real hook dry-run planner and exposes it via --plan, --why [STEP] (implies planning and can focus output), and --json (machine-readable plan output). The planner builds a structured Plan model (steps, parallel groups, dependencies, matched file counts, and inclusion/skip reasons) and renders it as either formatted text or pretty JSON.

Refactors shared logic for skip-step resolution and expression context building, updates CLI/settings snapshots to disambiguate hook-level --json from global trace JSON, and updates generated CLI docs/usage metadata. Adds extensive bats coverage for plan/why/json behavior, filtering, conditions, profiles, and regression cases ensuring commands are not executed during planning.

^{Reviewed by Cursor Bugbot for commit 109d363. Bugbot is set up for automated code reviews on this repo. Configure here.}

- `--plan` / `-P`: print a dry-run plan showing which steps would run, which would be skipped, and why — without executing any commands. - `--why [STEP]` / `-W`: show all reasons per step. Pass a step name to focus on one; omit the value to show details for every step. Implies `--plan`. - `--json` / `-J`: emit the plan as JSON (requires `--plan`) for tooling. The plan is built via static analysis (reusing `build_step_jobs` and `profile_skip_reason`) so it never executes step commands. It reports: step status (included/skipped), parallel group membership, dependencies, matched file count, and a structured list of reasons (filter match, profile include/exclude, condition true/false, CLI/env/config skip, missing required env, no command for run type). Removes the previous `--plan` stub that panicked via `todo!()`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

greptile-apps · 2026-04-19T01:34:37Z

Greptile Summary

This PR implements the --plan / -P, --why [STEP] / -W, and --json / -J flags for hk check/fix/run, replacing a prior todo!() stub. The planning path statically reuses build_step_jobs and profile_skip_reason to determine step inclusion/skip without executing any commands, and emits a human-readable or JSON plan. All three previously reported issues (profile-skip when job_condition is set, --why --json blocked by clap constraint, and --json alone silently falling through to hook execution) are correctly addressed in the final commits.

Confidence Score: 5/5

Safe to merge — planning path is read-only, all three previously flagged issues are resolved, and 13 bats tests plus 145 cargo tests pass.

All remaining findings are P2 or lower. The three P1 issues from prior review rounds (deferred-profile skip with condition, --json clap constraint blocking --why --json, and --json alone silently running the hook) are fully addressed. The analyze_step logic faithfully mirrors the runtime dispatch path and the test suite is thorough.

No files require special attention.

Important Files Changed

Filename	Overview
src/plan.rs	New data model for plan output — clean, well-typed structs with serde serialisation; no issues found.
src/hook.rs	Adds Hook::plan() and analyze_step(); deferred-profile + condition edge case explicitly handled; skip_reason_to_reason mapping complete.
src/hook_options.rs	--plan/-P, --why/-W, --json/-J flags added; runtime guard for --json without --plan/--why correctly exempts trace/HK_JSON/HK_TRACE env modes.
test/plan.bats	13 bats tests covering text+JSON output, profile gating, condition true/false, dependsOn, --step/--skip-step, no-execution invariant, and --why modes; comprehensive regression coverage.
src/step/job_builder.rs	Unchanged in this PR; confirms build_step_jobs always returns a job with skip_reason rather than an empty vec for no-files and profile-skip cases, which analyze_step relies on.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[analyze_step] --> B{step_condition?}
    B -->|Bool false| D["ConditionFalse → Skipped"]
    B -->|truthy / err| E[push reason, continue]
    B -->|not set| G[build_step_jobs]
    E --> G
    G -->|Err| H["Disabled → Skipped"]
    G -->|Ok| I{all jobs have skip_reason?}
    I -->|Yes| J["map SkipReason → Reason → Skipped"]
    I -->|No| K{job_condition?}
    K -->|Bool false| L["ConditionFalse → Skipped"]
    K -->|truthy / err| M[push reason, continue]
    K -->|not set| O{job_condition set AND profile_skip_reason?}
    M --> O
    O -->|Yes| P["ProfileExclude → Skipped"]
    O -->|No| Q["FilterMatch + optional ProfileInclude + CliInclude"]
    Q --> R["Included"]

_{Reviews (7): Last reviewed commit: "fix(check): prune group stepIds after --..." | Re-trigger Greptile}

gemini-code-assist

Code Review

This pull request introduces a comprehensive 'plan' feature to the CLI, enabling users to preview which steps will execute and the reasons for their inclusion or exclusion. Key additions include the -J --json flag for machine-readable output and the -W --why flag for detailed step analysis. The implementation involves a new plan module, logic in src/hook.rs to analyze step execution conditions, and updated documentation and integration tests. My feedback focuses on refactoring duplicated logic related to skip detection and expression context enrichment, as well as improving the handling of RunType string conversions and global progress state side effects.

gemini-code-assist · 2026-04-19T01:35:14Z

+        let skip_steps = {
+            let mut m: IndexMap<String, SkipReason> = IndexMap::new();
+            for s in env::HK_SKIP_STEPS.iter() {
+                m.insert(
+                    s.clone(),
+                    SkipReason::DisabledByEnv("HK_SKIP_STEPS".to_string()),
+                );
+            }
+            for s in settings.skip_steps.iter() {
+                if !m.contains_key::<str>(s) {
+                    m.insert(s.clone(), SkipReason::DisabledByConfig);
+                }
+            }
+            for s in opts.skip_step.iter() {
+                m.insert(
+                    s.clone(),
+                    SkipReason::DisabledByCli(format!("--skip-step {}", s)),
+                );
+            }
+            m
+        };


The logic for building the skip_steps map is duplicated across plan(), stats(), and run(). This should be refactored into a helper method to improve maintainability and ensure consistency when skip logic changes.

gemini-code-assist · 2026-04-19T01:35:14Z

+        let mut expr_ctx = EXPR_CTX.clone();
+        if let Ok(val) = expr::to_value(&git_status) {
+            expr_ctx.insert("git", val);
        }


The enrichment of the expression context with Git status is duplicated from the run() method. Consider extracting this into a helper method to avoid logic drift.

gemini-code-assist · 2026-04-19T01:35:14Z

+        let run_type_str = match run_type {
+            RunType::Check => "check",
+            RunType::Fix => "fix",
+        };


The conversion of RunType to a string is repeated in multiple places. It would be cleaner to implement a Display trait or an as_str() method on the RunType enum itself.

gemini-code-assist · 2026-04-19T01:35:14Z


    pub async fn plan(&self, opts: HookOptions) -> Result<()> {
+        // Suppress progress output so plan output (especially JSON) is clean.
+        clx::progress::set_output(ProgressOutput::Text);


Setting the global progress output mode to Text inside the plan() method is a side effect that persists after the method returns. While acceptable for a CLI tool that typically exits after one command, it could lead to unexpected behavior if hk is ever used in a context where multiple commands are executed in the same process. Consider using a scoped approach or resetting the output mode if necessary.

Correctness fixes (reported by reviewers on #848): - Mirror runtime profile-skip when `job_condition` is set. `build_step_jobs` defers profile checks to runtime when a condition is present (job_builder.rs:153) so the old plan reported such steps as Included. `analyze_step` now calls `profile_skip_reason` explicitly in that path. - Match runtime truthiness for `condition`. The runtime only skips on a literal `Bool(false)` — non-boolean values (e.g. strings from `exec()`) are truthy. Plan used `as_bool().unwrap_or(false)` which treated them as falsy and reported Skipped incorrectly. - Evaluate `step_condition` (was ignored). The runtime checks it in execution.rs before building jobs; the plan now mirrors that. Flag fix: - `--json` no longer requires `--plan` at the clap level, so `--why --json` works. Validated in code: errors with "--json requires --plan or --why" otherwise. Refactors: - Extract `build_skip_steps` and `build_expr_ctx` helpers used by `plan`, `stats`, and `run`. - Add `RunType::as_str()` and `Display` so the "check"/"fix" match is no longer duplicated. Adds five regression bats tests covering each correctness fix and the `--why --json` / `--json` without `--plan` cases. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

build_step_jobs populates job.files before applying skip_reason (see job_builder.rs lines 81-158), so profile-skipped steps still have a meaningful matched file count. The plan was hardcoding zero. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Replace `Ok(val) if val == expr::Value::Bool(false)` with the direct pattern `Ok(expr::Value::Bool(false))`. The guard-style match was the newly-introduced CI failure on `mise run lint`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

analyze_step pushed ConditionTrue first when a truthy condition passed, so print_plan's headline (reasons.first()) contradicted the icon when the step was later skipped for another reason (e.g. no files matched). print_plan now picks the first skip-flavored reason as the headline when status is Skipped; verbose --why still shows every reason. Adds ReasonKind::is_skip to classify reason kinds. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

`print_plan` picks the headline reason by finding the first skip-flavored entry, which may be at any index. The verbose detail loop used `.skip(1)`, so when the headline came from index > 0 it silently dropped the first reason and reprinted the headline. Track the headline's index and `continue` past it instead. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

The top-level CLI has a global --json flag for trace/log output. My prior validation (\"--json requires --plan or --why\") rejected \`hk --trace --json check\` because clap populates both the global \`args.json\` and the subcommand-level \`opts.json\` for the shared flag name. Remove the validation. --json alone continues to mean global JSON output (unchanged), and is only interpreted as plan-JSON when --plan or --why is also set (in which case plan() is entered). Drops the now-invalid regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…m variant - JSON output of \`--why <step> --json\` was ignoring the step focus and emitting every step. It now retains only the focused step (and its parallel group entry), matching the text renderer. - Remove unused \`ReasonKind::Always\` — never constructed. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit af48e83. Configure here.}

…e --json The previous commit dropped the validation to unbreak `hk --trace --json check`, but that made `hk check --json` silently run the hook with no JSON output. Reinstate the guard, gated on whether the user actually asked for tracing: if `--trace`, `HK_TRACE=json`, or `HK_JSON=1` is set, the global --json is legitimately active and we skip the error. Otherwise a bare `--json` without --plan/--why errors with "--json requires --plan or --why". Adds `CliSnapshot.trace` and `Settings::cli_trace()` to plumb the top-level --trace flag into HookOptions. Regression tests: both the error case and the `hk --trace --json` happy path are covered. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

`--why <step> --json` filtered `plan.steps` but left the retained group's `step_ids` pointing at siblings that were filtered out, producing dangling references for tooling that follows stepIds. Now also retain only the kept step names in each group and drop groups that become empty. Extends the regression test with a 3-step config so the group check actually has something to prune. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

@jdx