Skip to content

feat(when): expose shell.loop_kind for matching commands inside shell loops#345

Merged
fohte merged 3 commits into
mainfrom
fohte/add-shell-loop-kind
May 16, 2026
Merged

feat(when): expose shell.loop_kind for matching commands inside shell loops#345
fohte merged 3 commits into
mainfrom
fohte/add-shell-loop-kind

Conversation

@fohte
Copy link
Copy Markdown
Owner

@fohte fohte commented May 13, 2026

Purpose

  • Polling loops of the form until cmd; do sleep N; done should be deniable from a when clause, but runok currently exposes no way to tell that a sleep runs inside a loop, so a rule cannot distinguish a polling sleep from a standalone one
    • Even when a deterministic alternative exists (gh pr checks --watch, kubectl wait, ...), the only rule that can block the polling form today is a blanket deny on sleep N, which also blocks the legitimate standalone sleep calls that have to keep working

Approach

  • Stamp every extracted sub-command with the kind of shell loop that immediately encloses it, and expose that value to CEL as shell.loop_kind
    • Values: "while", "until", "for", or "" when the command is not inside any loop
    • Nested loops surface the nearest enclosing kind (sleep in for x in a b; do until y; do sleep 1; done; done reports "until")
    • Subshells do not reset the kind (sleep in (while x; do sleep 1; done) still reports "while")
    • Substitutions that evaluate once before the loop body keep loop_kind = "", whether bare or quoted (for i in $(cmd); do ...; done and for i in "$(cmd)"; do ...; done both report "" for cmd)
  • Example usage:
    rules:
      - deny: 'sleep *'
        when: 'shell.loop_kind in ["while", "until"]'
        message: 'Polling loops are fragile. Use --watch / wait flags or a native polling command instead.'
      - allow: 'sleep *'
Design decisions

CEL surface shape

Decision Design Pros Cons
Chosen shell.loop_kind: string, with "" as the loop-outside sentinel Matches the existing flat-map style of pipe.stdin etc.; in [...] works without has() gating Users have to learn that "" means "outside any loop"
Rejected Pair a shell.in_loop: bool field with the string in_loop reads naturally for the common case Redundant with loop_kind != ""; adds a field without enabling anything new
Rejected Make loop_kind nullable and check it with has(shell.loop_kind) Lets users rely on CEL's built-in absence check Diverges from the existing flat-map contexts (pipe.*, redirects[].*) that already use sentinel values rather than nullability

Loop kind for value-list substitutions

Decision Design Pros Cons
Chosen cmd in for i in $(cmd); do ...; done reports loop_kind = "" Matches bash evaluation order (the value list runs once before the loop body); bare $(cmd) and quoted "$(cmd)" agree Users who want to treat a value-list substitution as "loop-internal" need a different way to express that
Rejected Bare $(cmd) reports loop_kind = "for", quoted "$(cmd)" reports "" Picks up the substitutions that appear as direct AST children of the for-statement Behaviour splits across quoting, which is confusing for rule authors writing semantically identical commands

fohte added 3 commits May 13, 2026 20:41
…ll loops

intent(when): let `when` clauses tell `until cmd; do sleep N; done` from a bare
  `sleep N`, so polling-loop wrappers can be denied while standalone sleeps stay
  allowed
decision(parser): surface the loop kind as a single string `loop_kind`
  (`"while"`, `"until"`, `"for"`, or `""`) on every extracted sub-command and
  expose it via the existing flat CEL map shape (`shell.loop_kind`), mirroring
  `pipe.stdin` rather than introducing a `has()`-gated nullable
rejected(parser): a richer struct with `in_loop` / `has_timeout_wrapper` /
  `in_background` / `subshell_depth` / `loop_stack` — out of scope for the MVP,
  and `in_loop` is redundant with `loop_kind != ""`
learned(parser): tree-sitter-bash uses a single `while_statement` node for both
  `while` and `until`; the keyword shows up only as the leading anonymous token,
  so the kind has to be sniffed from the source bytes
constraint(parser): substitutions discovered through
  `collect_substitutions_recursive` (e.g. `for i in "$(cmd)"`) intentionally
  reset to `loop_kind = ""` because the substitution runs once outside the loop
  body, matching the existing pipe/redirect-context reset
… path

intent(adapter): make `shell.loop_kind` reach the entry path used by
  `for i in 1 2; do <single cmd>; done` (and any other loop whose body
  extracts to exactly one sub-command) — previously the single-command
  branch in the adapter called `evaluate_command_with_metadata` without
  threading `loop_kind`, so `when: shell.loop_kind == "for"` silently
  failed for the headline use case
decision(parser): drop `command_substitution` from the `for_statement`
  body branch and route value-list substitutions through
  `collect_substitutions_recursive` so bare `$(cmd)` and quoted
  `"$(cmd)"` value-list substitutions both stamp `loop_kind = ""`,
  keeping behaviour consistent with bash semantics (the substitution
  runs once before the loop body) rather than diverging by quoting
learned(parser): `collect_substitutions_recursive` only descends into a
  node's named children, so the `for_statement` arm has to keep an
  explicit `command_substitution` / `process_substitution` branch — the
  helper alone would silently miss a substitution that sits as a direct
  child of the for-statement
intent(docs): swap the `TODO(pr-link)` placeholder for the real PR
  link now that the PR has been opened
@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

❌ Patch coverage is 90.52632% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.05%. Comparing base (9dd0e7d) to head (5f7ced5).

Files with missing lines Patch % Lines
src/rules/command_parser.rs 89.28% 6 Missing ⚠️
src/rules/rule_engine.rs 90.47% 2 Missing ⚠️
src/adapter/mod.rs 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   89.07%   89.05%   -0.02%     
==========================================
  Files          53       53              
  Lines       12657    12731      +74     
==========================================
+ Hits        11274    11338      +64     
- Misses       1383     1393      +10     
Flag Coverage Δ
Linux 88.88% <90.52%> (-0.01%) ⬇️
macOS 90.19% <90.52%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new shell.loop_kind CEL variable to detect shell loops (while, until, for) within when clauses. This feature allows users to distinguish between polling loops and standalone commands, enabling more granular rule enforcement, such as denying sleep inside a loop while permitting it elsewhere. The implementation includes updates to the command parser, rule engine, and expression evaluator to propagate and expose this context, supported by comprehensive documentation and new integration tests. The reviewer correctly identified that the TODO(pr-link) placeholder in the release notes must be resolved before merging.

Comment thread docs/src/content/docs/releases/next.md
@fohte fohte merged commit 5b59604 into main May 16, 2026
13 checks passed
@fohte fohte deleted the fohte/add-shell-loop-kind branch May 16, 2026 09:47
@fohte-bot fohte-bot Bot mentioned this pull request May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant