feat(when): allow when clauses to check filesystem state via fs.* functions#341
Conversation
…em checks
intent(when): let `when:` clauses gate on the existence of an external
marker file (e.g. one written by a Claude Code skill) so a rule can
branch on out-of-process state, which the existing env / flags / args /
paths / redirects / pipe / vars / flag_groups / os surface cannot reach.
decision(when): collapse `NotFound` into `false` but surface every other
IO error (EACCES, etc.) as a CEL evaluation error — folding permission
failures into "false" would silently misclassify a missing-permission
parent directory as "marker absent" and quietly bypass the gate.
decision(when): register `fs` as a sentinel map and reject calls whose
receiver is not that exact map, so `add_function("exists", ...)` does
not turn `'foo'.exists('bar')` into a working call on arbitrary values.
constraint(when): cel-interpreter has no real namespace concept —
`fs.exists("p")` is parsed as a method call on a value `fs` with
function name `exists`, and registered functions live in a flat name
table. The 1-arg `exists` registration does not collide with CEL's
built-in `list.exists(v, pred)` macro because the parser dispatches the
2-arg comprehension form before reaching function lookup.
rejected(when): a separate `fs_exists` / `fs_is_file` / `fs_is_dir`
flat-named API — would force users to write `fs_exists("p")` instead of
the bash-shaped `fs.exists("p")` and diverge from how every other
context surface (`paths.X`, `flags.X`, `vars.X`) is spelled.
intent(release-notes): satisfy the project rule that next.md entries must carry a real PR link before merge.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #341 +/- ##
==========================================
+ Coverage 88.98% 89.03% +0.04%
==========================================
Files 53 53
Lines 12513 12580 +67
==========================================
+ Hits 11135 11200 +65
- Misses 1378 1380 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces new fs.* CEL functions (fs.exists, fs.is_file, fs.is_dir) to enable filesystem-based conditions in when clauses, complete with documentation and tests. The implementation includes logic to follow symlinks and properly handle I/O errors. Review feedback suggests optimizing the sentinel key lookup to prevent unnecessary allocations and refactoring the filesystem check implementations to reduce code duplication.
intent(when): address Gemini Code Assist review feedback on PR #341 -- the per-call `Key` allocation in `require_fs_target` was wasteful, and the `fs.is_file` / `fs.is_dir` bodies duplicated the same receiver check, empty-path short-circuit, and stat-error classification. decision(when): cache the sentinel `Key` in a `OnceLock` rather than a thread-local or `Lazy` -- `OnceLock` is in std (MSRV is fine on edition 2024), needs no extra deps, and the `Key` is read-only after init.
Purpose
when:clause can read is limited toenv/flags/args/paths/redirects/pipe/vars/flag_groups/os, with no access to live filesystem state at evaluation timegit commitonly when the pre-commit skill ran" -- gating on a marker file touched by a Claude Code skill -- cannot be expressed<path:name>placeholder is pattern matching on a command argument; it does not check the disk--checkis possible, but spawning a subprocess just for an existence test is overkillApproach
fs.exists(path)/fs.is_file(path)/fs.is_dir(path), callable fromwhen:clauses[ -e ]/[ -f ]/[ -d ]from the start, so the API does not drift in naming laterNotFoundtofalse, surface anything else (e.g.EACCES) as a CEL evaluation errorfalsewould letwhen: fs.exists(marker)silently misclassify a non-statable parent as "marker absent" and quietly bypass the gatefalsefrom all threefalse(not an error)Design decisions
Naming style
fs.exists/fs.is_file/fs.is_dir(snake_case)flag_groupssnake_case / mirrors bash's-fand-dintuitivelyfs.isFile/fs.isDir(camelCase)startsWithflag_groups); confusing to authorsI/O error handling
NotFoundtofalse; anything else becomes a CEL errorwhen:clause given an unstatable path stops evaluation with an errorfalsefs.*dispatchfsas a sentinel map; registerexists/is_file/is_diras functions with a receiver checkfs.exists("p")(consistent with other context surfaces) / misuse like'foo'.exists('bar')is rejectedfs_exists/fs_is_file/fs_is_dirfs_exists("p")syntax, diverging frompaths.X/flags.X/vars.X