fix: resolve correctness, security, and parsing bugs across codebase (#73)
* fix: resolve 10 bugs found during full codebase review
- Fix memory leak from Box::leak() in status bar render loop
- Fix AES-GCM nonce reuse vulnerability in encrypted secret storage
- Fix Default impl for EncryptedSecretStore that discarded encryption key
- Fix early return in list command that prevented GitLab pipeline listing
- Fix double validation call in validate_github_workflow
- Wire --show-action-messages CLI flag through ExecutionConfig
- Add serde(rename = "if") to GitLab Rule if_ field for correct deserialization
- Fix potential panic on multibyte paths in workflow tab path shortening
- Include involved job names in circular dependency error messages
- Improve cron syntax validation to check value ranges, steps, and expressions
* fix: address PR review feedback
- Remove dead `_nonce` parameter from `EncryptedSecretStore::from_data`
- Add clarifying comment for inverted show/hide action messages mapping
- Add comprehensive cron validation tests (valid expressions, out-of-range
values, wrong part count, invalid steps, invalid ranges, edge cases)
* fix: resolve 27 bugs found during full codebase verification
A thorough manual verification of every feature uncovered a
*remarkable* collection of bugs hiding in plain sight. The
highlights:
The `strategy.matrix` YAML structure was never parsed. The Job
struct had `matrix` at the top level, but GitHub Actions nests it
under `strategy.matrix`. Serde silently ignored the `strategy`
key, so matrix expansion code existed but could never run. For
absolutely no reason. Introduce a proper `Strategy` struct and
wire it through the executor.
The Step struct was missing `if`, `id`, `working-directory`,
`shell`, and `timeout-minutes` fields. Step-level conditionals
were silently dropped — every step always ran regardless of its
`if` condition. While at it, `continue-on-error` was in the
struct but had no serde rename and was never checked during
execution. Fix all of that.
The validator cheerfully reported cyclic `needs` dependencies as
"Valid". Add DFS cycle detection so `A -> B -> C -> A` is caught
at validation time instead of blowing up at execution time.
Five of eight GitLab CI test fixtures failed to parse because the
model was too rigid: `extends` only accepted arrays (not strings),
`variables` rejected integers, `cache.key` rejected structured
formats, and `script` rejected single strings. Add custom
deserializers following the existing codebase pattern.
The GitHub trigger function leaked the auth token via curl process
arguments visible in `/proc/[pid]/cmdline`. Replace with reqwest,
matching the pattern already used elsewhere. Also add symlink and
path traversal protections in the executor.
Other fixes: hardcoded matrix variable stripping replaced with
proper substitution, `show_action_messages` wired through TUI,
dead `if true {}` removed, default branch detection uses remote
HEAD instead of current branch, cron validator accepts named
days/months, reusable workflow ref validation loosened from OR
to AND, matrix include entries merge into all matching combos.
* fix: harden step-level evaluation, volume checks, and add tests
The PR review turned up a few things that needed fixing before this
was actually ready.
The step-level `if` condition evaluator was silently reusing the
job-level `evaluate_job_condition` function, which knows nothing
about step-scoped expressions like `steps.<id>.outcome`, `success()`,
`failure()`, `always()`, or `cancelled()`. These would fall through
to the generic "unknown condition" path without so much as a warning.
Now they're detected early, a warning is logged, and they default to
true — which is at least *honest* about the limitation.
The volume path traversal check (`..`) was applied to the entire
volume spec string, meaning a perfectly legitimate container path
like `/safe/host:/container/..weird` would get rejected. The check
now only inspects the host path component after splitting on `:`,
which is the part that actually matters for traversal attacks.
While at it, renamed the awkwardly-named `step_name_for_skip` to
just `step_name` in `execute_matrix_job` for consistency with
`execute_job`, and added a BREAKING_CHANGES.md documenting the
EncryptedSecretStore serialization format change.
Added 19 new tests covering matrix include/exclude merge semantics,
step condition evaluation for unsupported expressions, volume path
traversal edge cases, and continue-on-error + step-level if parsing.
* fix: correct condition defaults, path traversal check, and null variable handling
The previous commit defaulted *all* unsupported step-level
condition functions (failure(), cancelled(), always(), success())
to true. It turns out that defaulting failure() and cancelled()
to true is semantically wrong — it means steps guarded by
`if: failure()` will *always* run, even when nothing failed.
That's not a feature, that's a bug.
Default each function to its most likely state: always() and
success() return true, failure() and cancelled() return false.
Not perfect (we still can't track actual step outcomes), but at
least we're not silently running cleanup steps on every build.
The path traversal check was using `contains("..")` which is a
substring match. A directory literally named `..hidden` would
get rejected. Use Path::components() to detect actual ParentDir
components instead of playing string matching games.
While at it, fix deserialize_variables in the GitLab models to
handle YAML null values as empty strings instead of producing
"~\n". Also trim the catch-all serialization output.
* fix: correct cycle detection, condition evaluation, and matrix continue-on-error
The DFS cycle detector in `dfs_detect_cycle` had a genuinely nasty bug:
when a cycle was found, it returned early *without popping itself from
rec_stack*. This left stale entries that corrupted the stack for
subsequent DFS traversals. Net result: cross-edges to already-visited
nodes would be falsely reported as cycles. A→B→A is a cycle, but
D→E→A is just a cross-edge. The old code couldn't tell the difference.
Fix this properly by introducing a separate `in_stack` HashSet for O(1)
membership checks, while keeping the Vec for path reconstruction. Both
are now correctly cleaned up — no early returns skip the cleanup.
While at it, `execute_matrix_job` was silently ignoring `continue-on-error`
on the Err branch. The non-matrix `execute_job` handled it correctly,
but the matrix path would just abort the entire job. Copy-paste bugs
are fun like that. Let's fix that.
The `evaluate_job_condition` status function handling was doing sequential
`contains()` checks with early returns, which meant compound expressions
like `failure() || success()` would match `failure()` first and return
false. Now we scan for all status functions in one pass and pick the
most permissive default when positive functions are present.
Also: `convert_yaml_to_step` was hardcoding `None` for `if_condition`,
`id`, `working_directory`, `shell`, and `timeout_minutes` despite the
YAML potentially having them. And `is_valid_cron_atom` was rejecting
valid POSIX cron syntax like `5/2`.
* refactor(executor): extract step guards into shared helper, fix steps.* default
The step-level if-condition check and continue-on-error handling was
copy-pasted between execute_job and execute_matrix_job with subtly
different control flow — one sets job_success=false and breaks, the
other returns Ok(JobResult{Failure}) immediately. Two copies of the
same logic that *already* disagree is not redundancy, it's a bug
waiting to happen. Let's fix that.
Extract run_step_with_guards() that encapsulates the if-condition
evaluation, execute_step call, and continue-on-error wrapping into
a single StepOutcome enum. Both job execution paths now call this
shared helper.
While at it, fix the condition evaluator defaulting bare steps.*
references to true — "steps.build.outcome == 'failure'" should
*not* optimistically run the step. Now only always() and success()
default to true; everything else (bare step refs, failure(),
cancelled()) conservatively defaults to false.
Also add serde alias "matrix" on Job.strategy so old workflows with
flat matrix: at job level still parse, and document the intentional
or_insert_with in matrix include merging per GitHub Actions spec.
* fix: clean up review findings in step guards, secret store, and test fixture
The PR review flagged three issues worth fixing before merge.
First, run_step_with_guards had a bogus StepStatus::Skipped check
in the abort_job logic. The condition tested for Failure *or*
Skipped, then only actually aborted on Failure — meaning the
Skipped branch did nothing except confuse anyone reading the code.
Simplify to just check Failure directly.
Second, EncryptedSecretStore::from_json would silently fail with a
generic serde error when fed the old serialization format (which
had a shared top-level nonce field). Now it detects the old format
by checking for the "nonce" key and returns a clear error pointing
at BREAKING_CHANGES.md. Added a test for this.
Third, tests/workflows/continue-on-error-test.yml was an orphan
fixture — nothing referenced it. The same content is already
tested inline by parse_continue_on_error_workflow in the parser.
Removed it.
* fix: correct cron day-of-week range, steps. false positive, and Step boilerplate
Three issues from PR review, all straightforward:
The cron validator was rejecting day-of-week value 7, which is a
perfectly valid Sunday alias in both POSIX cron and GitHub Actions.
The max was 6 when it should be 7. The named-value resolver guard
also needed updating from `max == 6` to `max >= 6` so named days
still resolve correctly with the wider range.
The `evaluate_job_condition` heuristic for detecting `steps.*`
references was using a bare `contains("steps.")`, which means an
env var like `env.MY_STEPS_COUNT` would falsely trigger it and
short-circuit to false. Now we check that the character before
"steps." is either start-of-string or non-alphanumeric. Not a
full expression parser, but it stops the obvious false positives.
While at it, add a `Step::with_run` constructor so the GitLab
converter doesn't need three identical 12-field struct literals
that silently break every time someone adds a field to Step.
* fix: harden steps. boundary check, document condition semantics, dedup cycles
The steps. word-boundary heuristic in evaluate_job_condition was
checking for alphanumeric characters before "steps." to avoid false
positives on env vars like "env.MY_STEPS_COUNT". It turns out that
underscore is *not* alphanumeric, so "env._STEPS_CHECK" would
incorrectly trigger the step-reference path and return false.
While at it, the always() && failure() compound expression returning
true got a proper comment explaining *why* that's intentional — we
lack step-status context locally, so we'd rather over-run than
silently skip steps. Not ideal, but honest.
The DFS cycle detector in detect_cyclic_needs could report the same
cycle multiple times depending on HashMap iteration order. Normalize
cycles by rotating the node list to start at the lexicographically
smallest node, then deduplicate via a HashSet. Same cycle from
different entry points now gets reported exactly once.
* fix: squash review nits — double parse, clippy warnings, lost flag
Three leftover issues from the codebase review PR:
The from_json() deserialization was parsing the JSON *twice* — once
into serde_json::Value to sniff for the old nonce field, then again
from the raw string into the actual struct. Parse once, use
from_value() on the already-parsed Value. Not rocket science.
The cycle detector had two clippy warnings: .iter().cloned().collect()
on a slice (just use .to_vec(), please) and .min_by_key() cloning a
double reference instead of comparing properly. Switch to .min_by()
with an explicit cmp.
The show_action_messages flag was being silently dropped in
execute_workflow_cli — hardcoded to false regardless of what the user
asked for. Propagate it through the function signature and the TUI
fallback path so it actually does something.
2026-04-01 23:06:48 +05:30
# Breaking Changes
2026-04-22 00:21:09 +05:30
> The entries below shipped in v0.8.0.
docs: catch up on everything that landed after v0.7.3 (#107)
v0.7.3 was tagged back in August and then roughly fifty commits
happened. The docs, predictably, noticed none of this.
The README still advertised four TUI tabs when the TUI now has
seven, still listed three runtime modes when there are four, still
declared artifacts/cache/reusable-workflow outputs as "Not
Supported" when all three shipped in #88 and #94, and never
mentioned `wrkflw watch` or the `--event` / `--diff` /
`--changed-files` flags at all. `wrkflw-trigger-filter` and
`wrkflw-watcher` existed in the workspace without READMEs. Two of
the Rust examples referenced a `runtime` field on
`ExecutionConfig` that is actually called `runtime_type`, and
printed a `summary_status` field that doesn't exist. One
`run_wrkflw_tui` example was missing an argument. That kind of
thing.
While at it, BREAKING_CHANGES.md was labeling three entries as
"(v0.7.3)" when the underlying commits all landed *after* the
v0.7.3 tag — so calling them part of that release was, let's say,
a work of fiction. Relabel as "(Unreleased)" with a note up top
pointing at the next release.
New trigger-filter and watcher READMEs are deliberately short —
most users should hit that code through the CLI flags, not by
depending on the crates directly. No point padding them.
Nothing here is a code change. Just the docs finally telling the
truth about what's in the tree.
2026-04-21 23:43:11 +05:30
2026-04-22 00:21:09 +05:30
## `wrkflw run --event` requires change-set input by default (0.8.0)
feat: add diff-aware selective execution and watch mode (#91)
* feat: add diff-aware selective execution and watch mode
Add two new crates and CLI capabilities that differentiate wrkflw from act:
- `wrkflw-trigger-filter`: Parses workflow `on:` trigger configs (paths,
branches, tags filters) and evaluates them against git diff state to
determine which workflows would actually trigger.
- `wrkflw-watcher`: File system watcher using `notify` crate with
debouncing that auto-detects changes and re-runs affected workflows.
CLI additions:
- `wrkflw run --diff` uses git diff for trigger-aware execution
- `wrkflw run --event <type>` simulates a specific event type
- `wrkflw run --changed-files <list>` manually specifies changed files
- `wrkflw watch` monitors for changes and runs matching workflows
TUI additions:
- Press `d` to toggle diff-aware trigger filter overlay
- Shows green/gray indicators for which workflows would trigger
- Status bar reflects diff filter state
* fix(trigger-filter,watcher): correct glob semantics and fix watcher polling
The glob matchers in both path_matcher and ref_matcher were using
require_literal_separator: false, which means * happily matches /
in file paths and ref names. That's *not* how GitHub Actions works
— * matches anything except /, and ** is what crosses directory
boundaries. So src/* would incorrectly match src/sub/file.rs.
The test that was supposed to catch this (star_does_not_match_slash)
actually tested src/**/*.rs instead. Sneaky.
Set require_literal_separator: true in both matchers and fix the
test to actually assert the correct behavior.
While at it, fix two issues in the watcher's Debouncer:
The mutex handling was inconsistent — add_event and has_pending
silently swallowed poison, but drain called unwrap() and would
panic. Extract a lock_or_recover() helper that consistently
recovers from poison via into_inner() everywhere.
The main watch loop was busy-polling with a 100ms sleep, which is
wasteful and adds needless latency. Replace it with a
tokio::sync::Notify that fires when add_event inserts a path,
so the loop blocks properly until there's actual work to do.
* fix(trigger-filter): correct filter bypass and evaluate activity types
It turns out that when the event context had no branch (e.g., a tag
push), the branch filter check was simply *skipped*, meaning a
workflow with `branches: [main]` would happily match a tag push.
Same problem with the tag filter. The `if let Some(ref branch)`
guard meant "no branch = pass the filter", which is the exact
opposite of what GitHub Actions does.
The fix inverts the logic: check whether the filter *requires*
branches/tags first, and if the context doesn't have one, fail
immediately. While at it, the `types` field was being dutifully
parsed from YAML into `EventFilter.types` and then completely
ignored during evaluation. A workflow with `pull_request: { types:
[opened] }` would trigger on every PR event regardless. That's not
great. Add `activity_type` to `EventContext` and actually evaluate
it.
Also fix the TUI's `toggle_diff_filter` which was running
synchronous git subprocesses on the render thread — blocking the
entire UI while `git diff` thinks about its life choices. Move the
heavy lifting to `std::thread::spawn`. And replace an O(n²)
`Vec::contains` dedup in `get_changed_files` with a HashSet.
* fix(trigger-filter,watcher,ui): fix TUI blocking, watcher event loss, and dual filtering
Three issues found during review, all stemming from the diff-aware
execution feature introduced in the previous commits.
The TUI's toggle_diff_filter() was calling thread::spawn().join(),
which *blocks the main event loop thread* while git commands and
workflow parsing run. If git is slow (large repo, broken index,
network mount), the entire TUI freezes with no way out. Convert to
a fire-and-forget thread with an mpsc channel — the background
thread sends results, and check_diff_filter_results() polls on each
tick via try_recv(). No blocking, no freezing.
The watcher's main loop had a subtle event loss bug. After
notify.notified().await returns and a (potentially long) workflow
execution completes, any events that accumulated during execution
sit in the debouncer. But the loop goes back to notified().await,
which blocks until the *next* filesystem event — the accumulated
ones are orphaned. Add a has_pending() check before awaiting so we
process the backlog immediately.
The executor had an event_filter field on ExecutionConfig, giving it
trigger evaluation responsibilities that don't belong there. The
watcher already filters externally (correctly), creating two
divergent code paths for the same concern. Remove event_filter from
ExecutionConfig entirely and move trigger evaluation to the call
site in main.rs, where it belongs. The executor should just execute.
* fix(trigger-filter,watcher,ui): eliminate duplicated git logic, fix missing untracked files, and parallelize watcher
It turns out the TUI's evaluate_diff_filter was hand-rolling its
own git commands via std::process::Command instead of reusing the
async helpers in trigger-filter::git. Worse, it was only running
`git diff --name-only HEAD` and *not* including untracked files
via `git ls-files --others`. So newly added files were silently
invisible to the TUI diff filter. Not great.
Add synchronous wrappers (get_changed_files_sync,
get_current_branch_sync, get_current_tag_sync) to
trigger-filter::git, sharing the parsing logic with the async
versions via extracted helpers. The TUI now calls these instead
of maintaining its own parallel universe of git invocations.
While at it, fix three more things:
- The watcher was executing triggered workflows sequentially,
meaning a slow workflow would block all others and starve
event processing. Switch to futures::future::join_all for
concurrent execution.
- The debouncer's drain() was unconditionally sleeping for the
full debounce duration even when nothing was pending. Now it
returns immediately when empty and re-checks for new arrivals
after each sleep window so event bursts settle properly.
- The eval reason messages were useless when filters failed due
to missing context (no branch, no tag, no activity type). They
now say *why* the filter couldn't match instead of just dumping
the pattern list.
* fix(watcher,trigger-filter,ui): fix watcher stalls, unbounded concurrency, and sync API duplication
The watcher had a few problems that would make it miserable to use on
any real-world repository.
First, the notify callback was using blocking_send() on the mpsc
channel, which means when the channel fills up (256 slots), the *OS
filesystem event thread* blocks. That's the kind of thing that makes
your entire watcher stop receiving events. Switched to try_send() —
dropping a duplicate event is fine since the debouncer coalesces
anyway.
Second, the watcher was recursively watching the entire repo root
with no exclusions beyond .git/. On any project with a target/ or
node_modules/ directory, this floods the event channel with thousands
of irrelevant build artifact changes. Added a DEFAULT_IGNORE_DIRS
list that filters at the path-component level.
Third, join_all() on the triggered workflow futures meant *unbounded*
concurrent execution. If 20 workflows trigger simultaneously, that's
20 Docker containers spawning at once. Replaced with
buffer_unordered(4) so we don't eat all available resources.
While at it, a few more cleanups from the review:
- Renamed the misleading on_event callback to on_cycle_complete,
since it fires *after* execution completes, not on detection.
- Extracted the 170-line evaluate-and-execute loop from run() into
its own evaluate_and_execute() method.
- Parallelized the two sequential git subprocess calls in
get_changed_files() with tokio::join!.
- Killed the entire sync API in git.rs (three copy-pasted functions).
The TUI's evaluate_diff_filter now spins up a lightweight
current_thread tokio runtime instead of maintaining duplicate sync
wrappers. Having the same logic in two places with two slightly
different implementations is not redundancy, it's a bug farm.
- Renamed LogFilterLevel::to_string() to as_str() because shadowing
the ToString trait with an inherent method is a Clippy lint and
genuinely confusing.
* fix(trigger-filter,watcher,ui): fix debouncer livelock, dead code, and broken diff base
Three issues found during review, all interconnected:
The debouncer's drain() loop had no upper bound on settle rounds.
If filesystem events arrived faster than the debounce window (e.g.
a large build writing hundreds of files), it would spin forever
comparing pending.len() != count_before. Add a MAX_SETTLE_ROUNDS
cap of 10 so it eventually gives up waiting for silence and just
drains what it has.
The trigger-filter crate exported filter_workflows() and
auto_detect_context() — nice reusable helpers — and then both
state.rs and watcher.rs proceeded to ignore them entirely and
inline their own copies of the same logic. Three copies of "build
EventContext, loop over workflows, evaluate triggers" is not
redundancy, it's a maintenance trap. Refactor both callsites to
actually use the helpers they're sitting next to.
The TUI diff filter was hardcoded to diff against HEAD, which only
shows uncommitted changes. Post-commit, the diff is empty and every
workflow gets filtered out. Add get_default_diff_base() which
checks for uncommitted changes first (uses HEAD), then falls back
to merge-base with main/master, then HEAD~1. The TUI now uses this
via auto_detect_context so it shows meaningful results even after
committing.
While at it, cargo fmt touched eval.rs formatting. No logic change.
* fix(trigger-filter): fix staged file blindness, hardcoded default branch, and dead code
get_changed_files() was running `git diff --name-only HEAD` and
proudly calling that "staged + unstaged changes." It turns out
that command only shows *unstaged* changes. Files sitting in the
index after `git add` were completely invisible to trigger
evaluation. Confusion ensues.
Add a parallel `git diff --cached --name-only <base>` call and
merge its results with the unstaged diff, deduplicating paths.
Now we actually see what the user staged. Novel concept.
get_default_diff_base() had "main" and "master" hardcoded as the
only candidates for the merge-base. Repos using "develop", "trunk",
or literally anything else would silently fall back to HEAD~1,
which is almost certainly wrong. Query `git symbolic-ref
refs/remotes/origin/HEAD` first to detect the *actual* default
branch, then fall back to the hardcoded list.
While at it, remove the dead PatternError variant from
TriggerFilterError — it was never constructed anywhere.
* fix(trigger-filter,watcher,ui): consolidate EventContext, fix fallback crash, and harden watch loop
Three call sites — CLI run, TUI toggle, and the watcher — were each
hand-assembling EventContext with subtly different logic. The CLI
forgot to use auto_detect_context, the watcher skipped untracked
files, and the TUI was the only one doing it right. It was only a
matter of time before these diverged further.
Add context_from_diff_range() and context_from_changed_files() to
trigger-filter and use them everywhere. One place to get it right,
zero places to get it wrong.
The get_default_diff_base() fallback was blindly returning "HEAD~1"
which *explodes* on single-commit repos because there is no parent.
Now we verify HEAD~1 resolves first and fall back to the git empty
tree SHA (4b825dc6...) as the ultimate backstop — which means "all
files are changed" rather than "crash and burn."
While at it:
- Wrap on_cycle_complete in spawn_blocking so a blocking callback
can't stall the entire watch loop
- Re-collect workflow files each cycle so newly added .yml files
get picked up without a restart
- Guard rapid 'd' toggle in the TUI against spawning duplicate
background threads by dropping the in-flight receiver first
- Add 15 new tests covering filter_workflows, should_ignore_path,
debouncer livelock prevention, invalid globs, and parser types
* fix(watcher,wrkflw,ui): make watch concurrency configurable and warn on lone --event
Three small things from a code review that were bugging me.
First: the watcher had MAX_CONCURRENT_EXECUTIONS hardcoded to 4.
Fine for a typical .github/workflows directory, terrible for anyone
with 30 workflows who actually wants them to run. Promote it to
DEFAULT_MAX_CONCURRENT_EXECUTIONS, plumb a max_concurrency parameter
through WorkflowWatcher::new, and expose it as --max-concurrency on
the watch command. Clamp to >= 1 because passing 0 to
buffer_unordered would just deadlock, and "deadlock on user input"
is not the user experience I want to ship.
Second: `wrkflw run --event push` (without --diff or
--changed-files) silently builds an EventContext with zero changed
files. Any workflow with a `paths:` filter then "doesn't trigger"
and the user has no idea why. Log a warning explaining exactly what
happened and what to do about it. Not a fix per se — the behavior is
technically correct — but silent surprise is worse than a loud
warning.
While at it: add a comment on the `_watcher` binding in the watch
loop because RecommendedWatcher stops emitting events the moment it
gets dropped, and that's the kind of footgun future-me will
absolutely step on. Also flatten a pointless nested if/else in
status_bar.rs that clippy would have eventually complained about
anyway.
* fix(trigger-filter,watcher,ui): stop lying about which workflows would run
The diff-aware execution feature had a pile of quiet ways to
mislead the user, which is the worst kind of bug in a tool whose
entire purpose is "tell me what would run."
First, invalid glob patterns were silently swallowed. A typo
like `paths: [src/**.rs]` matched nothing and there was no
diagnostic — the workflow just reported "would not trigger" and
everyone assumed it was correct. Compile globs up front in
parse_trigger_config and surface bad patterns as ParseError
pointing at the exact key (e.g. `push.paths`). Pre-compiling
also means we're not re-running Pattern::new on every hot path.
Second, the \`pull_request: branches:\` filter was matched against
the current working branch. GitHub Actions matches it against
the *base* (target) branch, so any workflow guarded by
\`pull_request: branches: [main]\` was reported as "would not
trigger" when evaluated on a feature branch. That's the exact
opposite of what happens on GitHub. Add base_branch to
EventContext, route it through branch_for_filter(), expose a
--base-branch flag on both \`run\` and \`watch\`, and warn loudly
when the user simulates pull_request without it.
Third — and this is the fun one — the TUI's diff filter zipped
background-thread results *positionally* against self.workflows.
The background thread captured workflow_paths at toggle time
and sent back a Vec. If the workflow list reloaded between
toggle and result delivery (a new .yml appears on disk, which
is *exactly* what a file-watcher app does), the positional zip
assigned statuses to the wrong workflows. Send (PathBuf, status)
pairs through the channel and key them by path on receipt.
While at it:
- should_ignore_path matched any component anywhere in the path
against the blacklist, so a file literally named
\`scripts/target\` or \`docs/build\` was silently dropped.
Repo-root-relative + skip-the-leaf fixes it.
- All git commands take an optional cwd and prepend
\`git -C <root>\`, so the watcher queries the repo it's actually
watching instead of whatever the process CWD happens to be.
- Ref names are validated against an allowlist before being
passed to \`git diff\`, so \`--upload-pack=foo\` and friends can't
sneak in through --diff-base.
- For non-HEAD base, the redundant \`git diff --cached <ref>\` was
doing nothing useful — \`git diff <ref>\` already covers staged
+ unstaged. Dropped.
- Workflow files are cached in the watcher and only reparsed
when they actually change. If all of them fail to parse, say
so — before, the user just saw "0 triggered, 0 skipped" and
wondered what they'd broken.
- Empty-tree SHA fallback is now a named constant and logs a
warning when it's used. If you've fallen back to diffing
against the empty tree, you almost certainly don't want to.
- The 200-line Run arm in main.rs is extracted into
run_trigger_prefilter_or_exit().
- Dead TriggerFilterError::IoError variant removed.
Tests: 57 in trigger-filter (incl. PR base_branch semantics,
invalid-glob parse errors, ref validation), 13 in watcher
(incl. file-named-target cases), plus a regression test in the
TUI for the path-keyed mapping.
* fix(watcher,trigger-filter): stop dropping fs events and thrashing globs
A fresh code review turned up a handful of real issues in the
diff-aware execution path. A few of them were silent failure
modes, which is the worst kind.
The watcher had a 256-slot bounded MPSC with `try_send` sitting
between the notify callback and the debouncer. The comment said
dropping was fine "because the debouncer coalesces anyway" — which
is only true for repeat events on the *same* path. Burst 300
distinct file changes at it and ~44 unique paths just vanish, and
any workflow whose `paths:` filter matched exactly one of those
files fails to trigger. Which is the entire point of the feature.
Rip the channel out. The notify callback now pushes straight into
the debouncer's shared set — a hashset insert under a mutex,
bounded in cost, and it dedups for free. No intermediate buffer
to overflow.
It turns out `strip_prefix(&self.repo_root)` was also lying to us.
On macOS, `/var` gets delivered as `/private/var`; symlinked
worktrees have the same problem. Every event would fail the
prefix check, the relative path list ended up empty, and the loop
cheerfully `continue`d without telling anyone. Canonicalize the
repo root once at startup and canonicalize incoming paths before
stripping. Warn in verbose mode when events still don't resolve,
so there's a breadcrumb when it does go wrong.
While in there, the watcher was re-parsing every workflow's
trigger config *and recompiling every glob pattern* on every
cycle, because `filter_workflows` takes `WorkflowDefinition` and
parses on the spot. Add `filter_trigger_configs` taking pre-parsed
`WorkflowTriggerConfig`s, and have the watcher cache them by path,
invalidated only when the backing file actually shows up in
`changed_paths`. Free performance for anyone with more than three
workflow files.
And some smaller stuff the review turned up:
`auto_detect_context` and friends were running three git
subprocesses sequentially for absolutely no reason. `tokio::join!`
them. `validate_ref_name` was rejecting `HEAD@{1}`, `@~1`, and
`origin/main@{upstream}` — all perfectly legitimate git revision
syntax — so extend the allowlist to include `@`, `{`, `}`. The
TUI's diff-filter evaluator was hardcoding `event_name = "push"`;
thread it through as a parameter so future event selection is a
plumbing change, not a rewrite. And `run --diff` crashed with a
confusing "Error parsing workflow" message when pointed at a
directory — catch that up front and point the user at
`wrkflw watch` instead.
* fix(trigger-filter,watcher,ui): plug silent-failure holes and unblock reactor
The review on this branch turned up a handful of "the filter
quietly lies to you" bugs plus a watch loop that cheerfully
calls blocking syscalls from inside an async function. Let's
fix all of it.
First, the *actually* scary one: inline `!`-prefixed patterns
in `branches:`/`tags:`/`paths:` were compiled as literal globs.
The `glob` crate doesn't treat a leading `!` as negation, so
something like `branches: ['main', '!release/old']` would
compile `!release/old` into a pattern that matches the string
`!release/old` and *nothing else*. The exclusion got silently
dropped and the workflow happily triggered on `release/old`.
Route those into the `*_ignore` lists at parse time, support
the `!!literal` escape, and reject mixing inline `!` with a
dedicated `*-ignore` key (GHA itself rejects that). Four new
tests cover it.
Second silent lie: `get_default_diff_base` fell back to the
empty-tree SHA when no better base could be detected, which
made `git diff` list *every tracked file* as changed. The
"filter" then matched everything — the log warning was the
only hint. Make it return an error and let the CLI tell the
user to pass `--diff-base` explicitly. A filter that can't
tell you what it's filtering is worse than no filter.
While at it, wrap every git subprocess in a 10s
`tokio::time::timeout` so a hung git (network fs, credential
prompt, corrupt repo) can't wedge the watch loop forever. And
drop the redundant `git diff --cached HEAD` pass — plain
`git diff HEAD` already covers staged + unstaged + deleted.
It was doing nothing but spawning a second process.
Third problem was the watch loop itself. `collect_workflow_files`
ran `read_dir` synchronously *inside* the async run loop on
every cycle, and the per-cycle canonicalize pass called
`std::fs::canonicalize` on every changed path without ever
hopping onto a blocking thread. On a branch switch touching
thousands of files this stalls the reactor. Move both onto
`spawn_blocking`. Also rewrote `should_ignore_path` to walk
`rel.parent().components()` directly instead of collecting
into a `Vec<Component>` — it runs on every notify event, so
the allocation was genuinely wasteful.
The doc comment on `run()` claimed the callback "will not
stall the async watch loop" because it ran on `spawn_blocking`.
That's misleading — the loop awaits the spawned task, so the
*next* cycle doesn't start until the callback returns. Rewrote
it to describe the actual behavior (events accumulate in the
debouncer during the callback; keep it fast).
While I was in the UI crate, the diff-filter toggle was
spawning a raw OS thread and building a fresh
`current_thread` tokio runtime on it — inside a TUI that's
*already* running on a tokio runtime. This is the kind of
thing that looks fine until you try to debug it. Use
`tokio::task::spawn` on the ambient runtime and push the
workflow-parsing loop onto `spawn_blocking`. Same amount of
concurrency, none of the nested-runtime weirdness.
Also: the TUI hardcodes `push` as the simulated event but
never told the user. A workflow with `on: pull_request` would
show up as SKIPPED and the user had no idea why. Surface it
in the block title as `[DIFF: push]` and mention it in the
toggle log. Extracted to a `const` on `App` so a future event
selector is plumbing-only.
Minor cleanups: consistent `Error:` prefixes in the CLI
prefilter (was mixing "Warning: Failed" with `exit(1)` which
is just confusing), and dropped the now-unused
`wrkflw-logging` dep from `trigger-filter`.
All workspace tests pass, clippy is clean.
* fix(watcher,trigger-filter,ui): plug deleted-file + detached-HEAD holes and kill TUI foot-guns
A principal-engineer review of this branch turned up a pile of
correctness bugs and UX foot-guns that were all individually small
but collectively embarrassing. Let's fix them.
The nastiest one: when a file was deleted, `canonicalize` would fail
on the missing target, the fallback was the raw path from notify, and
on macOS (`/private/var` vs `/var`) or any symlinked working tree the
subsequent `strip_prefix(repo_root)` would drop the event on the
floor. So `rm src/main.rs` would *silently* fail to trigger any
workflow with `paths: [src/**]`. Which — in a tool whose entire point
is "run the workflows for what actually changed" — is not great.
Fixed by walking back to the nearest canonicalizable ancestor and
re-joining the missing tail. It's not pretty, but it's correct, and
it keeps deletions on the happy path.
While at it:
- `should_ignore_path` used to iterate the *absolute* path
components when `strip_prefix` failed, which means a perfectly
innocent `/home/alice/target-acquisition/file.rs` would match the
`target` ignore entry. Now we just refuse to filter anything that
isn't under the watch root. Please don't have directories named
after cargo internals outside your repo, but if you do, we won't
punish you for it.
- `get_current_branch` was returning the literal string `"HEAD"` in
detached-HEAD state, because that's what `git rev-parse
--abbrev-ref HEAD` prints. Any workflow with `branches: [HEAD]`
would then "match" in detached state. Signature is now
`Result<Option<String>, _>`, callers `.ok().flatten()`.
- Notify events are now filtered by kind. Access, metadata, Any,
Other are dropped; only Create, Remove, Modify::Data,
Modify::Name make it to the debouncer. A `git checkout` firing a
thousand rename events would previously blow through
MAX_SETTLE_ROUNDS mid-burst. It still might, but at least we're
not also counting every atime update.
- The run() callback is now fire-and-forget. Previously the watch
loop awaited `spawn_blocking(cb).await`, so a slow reporter (file
sink, Slack webhook, whatever comes next) would stall the next
cycle. Documented as "keep it fast" is not a real defense.
On the TUI side, the diff-filter key (`d`) had three separate
problems.
First, the event was a hardcoded `const DIFF_FILTER_EVENT = "push"`.
Users whose main workflow is `pull_request` would press `d`, see
every single workflow reported as SKIPPED, and file a bug. It's now
a field (`App::diff_filter_event`) so a future event selector is a
data-flow change and not a public API churn.
Second, rapid toggles leaked background tasks. Each press spawned a
fresh git + parse-every-workflow task and just dropped the previous
channel. Now we hold a `JoinHandle` and `abort()` the previous task
before starting a new one.
Third, when `auto_detect_context_default_base` errored (fresh repo,
no default branch, whatever), the UI silently showed every workflow
as None and the summary line said "0/N workflows would trigger" —
with *no* indication that anything had failed. The background task
now returns a `DiffFilterOutcome::{Success, Failure}` and the
failure reason gets pushed into the TUI log.
Also:
- Status bar default hint lost `t trigger` when `d diff` was added.
The `t` keybinding still worked, only discoverability was gone.
Restored.
- Workflows-tab title now reads the dynamic `diff_filter_event`
field instead of the const.
Structural cleanup that was overdue anyway:
- Introduced `WatcherConfig` builder. `WorkflowWatcher::new` already
had eight parameters and a `#[allow(clippy::too_many_arguments)]`
— the next knob was going to push it past ten. The old positional
constructor is kept as a legacy forwarder so the CLI call site
doesn't have to change in the same commit.
- Split `WorkflowWatcher::run` into `refresh_trigger_cache` and
`canonicalize_changed_paths` helpers. The loop was 190 lines of
five concerns inline and completely untestable without spinning
up a real notify watcher.
- New `wrkflw_trigger_filter::load_trigger_config(path)` helper
consolidates read + parse + compile globs. The watcher, the TUI
diff evaluator, and the `run` CLI prefilter now all go through
the same function, so they fail identically on the same broken
file instead of each printing its own flavor of error.
- `parser::resolve_include_and_ignore` replaces 40 lines of near-
identical copy-paste across `branches`/`tags`/`paths`. Having
the same bug in three places is not my idea of redundancy.
- `wrkflw run` prefilter now roots git operations at the repo root
via `find_repo_root()`, matching what the watcher already does.
Consistent behavior when invoked from a subdirectory.
Tests: 12 new tests covering deleted-file canonicalization, event-
kind filtering, detached HEAD, diff-base error path, and the
failure-reporting path in the TUI. Tempfile-backed git tests for
`get_changed_files`, `get_default_diff_base`, and
`get_current_branch`. Regression test for the absolute-path leak in
`should_ignore_path`. 104 tests total across the three crates, all
green, clippy clean.
* fix(watcher,trigger-filter,ui,wrkflw): plug stale cache, silent parse failures, and reactor stalls
- watcher: refresh_trigger_cache compared raw notify paths (absolute,
OS-canonicalized) against read_dir paths (relative), so the
"this workflow file was edited, reparse it" branch was dead code and
watch mode served stale parsed configs forever after the first edit.
Canonicalize both sides into the same form before set membership.
- watcher: refresh_trigger_cache ran sync file I/O on the reactor.
Move it onto a spawn_blocking thread via a new async wrapper; keep
the sync helper as a free function so unit tests don't need a runtime.
- ui: evaluate_diff_filter dropped parse failures via .filter_map(.. .ok()),
leaving users with N workflows showing as "-" and no signal that
their YAML was broken. Capture failures into a new DiffFilterReport
and surface each one in the log pane.
- ui: evaluate_diff_filter ran git helpers with cwd: None, so the TUI
evaluated against whatever directory the user happened to be in
rather than the discovered repo root. Plumb find_repo_root() through.
- trigger-filter: validate_ref_name accepted '..', git's range syntax,
which slipped through into format!("{base}..{head}") interpolation
and produced confusing three-dot errors. Reject it explicitly.
- trigger-filter: move find_repo_root from watcher into trigger-filter
(the right home for our git shell-outs); watcher re-exports it for
source compat.
- wrkflw,watcher: migrate the CLI off the legacy WorkflowWatcher::new
positional ctor to the WatcherConfig builder, then delete the legacy
ctor and the unused filter_workflows API.
* fix(trigger-filter,watcher,wrkflw): plug git zombie + watcher panic + silent filter holes
PR review on the diff-aware/watch branch turned up six issues that
either silently lie to the user or quietly degrade the watch loop
over time. None of them blow up immediately, which is precisely why
they're worth fixing before this lands.
It turns out the 10s `tokio::time::timeout` around every git
subprocess does *nothing* for the failure mode it was built for. A
hung child (NFS stall, credential prompt, corrupt repo) keeps
running after the timeout fires because `tokio::process::Command`
does not enable `kill_on_drop` by default — the future just gets
dropped and the zombie keeps consuming a process slot. On a long-
running watch session against a flaky mount that's exactly how you
end up with hundreds of zombie `git` processes. `kill_on_drop(true)`
on `git_cmd`. One line. Should have been there from the start.
The parser was happy to accept `paths:` + `paths-ignore:` (and the
branches/tags equivalents) on the same event. GitHub Actions rejects
that combination at upload time, so users could iterate locally
against semantics that would later fail in production. Same crate,
`extract_string_list` was silently dropping non-string list entries
via `filter_map(... .as_str())` — a typo like `paths: [{src: foo}]`
yielded an empty list, which then matched everything. The exact
"silently lying about which workflows would run" failure mode the
rest of this branch is built to prevent. Both now surface as
`ParseError` with the offending `event.key[idx]` location.
`auto_detect_context` collapsed a real `git rev-parse` failure to
`branch: None` via `.ok().flatten()`, which made a permission denied
on `.git/HEAD` look identical to a clean detached checkout. The
detached-HEAD path already returns `Ok(None)` from
`get_current_branch`, so the fix is just to drop the `.ok().flatten()`
and let `?` propagate real errors.
The watcher had two ways to silently brick itself. First, when
`context_from_changed_files` errored, it fell back to a context
with `branch: None` and kept going — every `branches:`-filtered
workflow then deterministically rejected for the rest of the session
while the user just saw "0 triggered" forever. Now the cycle short-
circuits, attaches the reason to a new `WatchEvent::error` field,
and the CLI surfaces it. Second, a panic from the executor would
unwind through `buffer_unordered` and kill the entire watch loop —
one rogue workflow takes down the session for everything. I wanted
to use \`tokio::spawn\` for isolation, but the executor's
\`dyn ContainerRuntime\` is not \`Sync\` so the future is not \`Send\`.
\`AssertUnwindSafe(...).catch_unwind()\` from \`futures\` works on the
local future, doesn't require \`Send\`, and gives us the same
"contain it, log it, keep going" behavior.
While at it: cache the canonical workflow path inside each trigger
cache entry so refresh stops re-canonicalizing every workflow on
every cycle (one \`lstat\` walk per file per cycle adds up on a
network mount); drop the no-op \`WorkflowWatcher\` accessor methods
that were pretending to be source-compat shims on a brand-new file;
and split the 130-line, 8-arg \`run_trigger_prefilter_or_exit\` in
main.rs into a \`PrefilterRequest\` struct + two helpers so the
\`#[allow(clippy::too_many_arguments)]\` can go away.
Tests cover all of it: parser combo-rejection (paths/branches/tags),
non-string entry rejection in both \`paths:\` and \`types:\`, the null-
is-empty negative case, canonical-path memoization across two
refresh cycles, and a structural test that
\`AssertUnwindSafe.catch_unwind\` actually classifies a panicking
future as \`Err\` so the watcher's match arm fires. Existing
\`parse_mapping_with_tags\` rewritten to use inline \`!\`-negation
because the old shape now violates the rule it asserts against.
\`cargo test --workspace\`: zero failures. clippy + fmt clean.
* fix(trigger-filter,watcher,ui): kill silent failures and dead code
found in review
A principal-engineer-style review of the diff-aware-execution branch
turned up a small pile of paper cuts. None of them are catastrophic
on their own, but together they're the kind of "everything looks
fine, why is nothing triggering?" failure mode this entire feature
is supposed to *prevent*.
The main offender lives in `extract_glob_list`. It carried a
`compiled.source = display_src;` reassignment with a comment claiming
it preserved the human-visible source for the `!!` escape — except
`pattern_src` and `display_src` are equal in every branch, and
`GlobPattern::new` already wrote `pattern_src` into `compiled.source`
on the line above. So the assignment did nothing, and the comment
lied about it. Collapse the two variables into one `pattern_str`
and delete the dead write. While at it, document the greedy `!!`
behavior properly and pin the `!!!foo` triple-bang case with a
test, since GHA's spec doesn't cover it and we *will* refactor this
function again someday.
The TUI's `toggle_diff_filter` was calling `find_repo_root()`
synchronously on the UI thread. That's a `git rev-parse
--show-toplevel` subprocess. Microseconds on local disk, an
*actual hitch* on a network mount, every single toggle. Move it
into `spawn_blocking` inside the spawned task where it belongs.
`evaluate_and_execute` was also doing two pointless `git`
subprocess calls per cycle when there were zero parseable
workflows. Short-circuit on `configs.is_empty()` and stop paying
for context we'll never use.
While I was in there:
- `canonicalize_changed_paths` used to `filter_map(... .ok())`
away any path it couldn't strip-prefix to the repo root.
Notify *should* never deliver one of those, but if it does,
silently dropping the event is exactly the failure mode this
feature exists to kill. Log the dropped path under `--verbose`.
- `get_default_diff_base` only stripped the `origin/` prefix
from `git symbolic-ref --short` output. If `--short` ever
fails to apply (older git, weird vendor, who knows), the
output is `refs/remotes/origin/main` and the candidate name
ends up wrong. Strip both forms.
- `get_changed_files`'s doc claimed "covers staged + unstaged
+ committed-on-branch." That's *almost* true, but the
modify-then-stage-then-revert-in-WT edge case is genuinely
invisible to `git diff <base>`. Don't promise things we can't
deliver — the trigger filter is an approximation, not a full
index audit.
- `explain_filter_failure` now mentions an empty change set when
a `paths:` filter rejects, so the user who passed `--event
push` without `--diff` gets a directly actionable hint instead
of a `paths: ["src/**"]` dump and a furrowed brow.
- `WorkflowWatcher::run` is an unbounded `loop {}` with no
cancellation path. The CLI fakes graceful shutdown by calling
`process::exit(0)` from a Ctrl+C handler. That works for the
CLI, but anyone embedding this in a daemon or test harness is
going to be unhappy. Document the limitation loudly so the
next person knows it's a known follow-up rather than an
oversight.
- The `JoinHandle::abort` in `toggle_diff_filter` doesn't
actually cancel anything — abort only signals at await
points, and the git futures inside the task may keep running.
Soften the doc to "no longer observed" instead of overstating
the guarantee.
None of this is glamorous. The bias of this entire branch has
been "fail loud, don't lie about what's running," and this just
finishes the cleanup the review surfaced.
Build clean, all 71 trigger-filter / 21 watcher / 19 ui tests
pass, no clippy warnings.
* fix(trigger-filter,watcher,ui,wrkflw): kill five more silent-failure holes from review
A principal-engineer review of the diff-aware/watch branch turned up
five places where this PR was still doing the exact thing the rest of
the branch was built to stop doing: failing quietly. Let's fix them.
`merge_unique` in trigger-filter built a `seen` set from `into` and
then never updated it inside the loop, so duplicates *within* `more`
itself happily slipped through. The function name is a contract — it
just wasn't being honored. Switch to `HashSet::insert`'s "was new"
return so each entry costs one lookup, not two, and add the obvious
regression test.
The watcher's notify callback was the next offender: a stray
`if let Ok(event) = res` swallowed every `notify::Error` — queue
overflow, inotify watch budget exhaustion, kernel disconnects, the
lot. The user gets a cheerful "watching..." banner while the loop is
effectively dead. Log the errors as warnings instead. While at it,
`canonicalize_changed_paths` was doing the same trick with
`unwrap_or_default` on its `JoinError`, mirroring nothing the
neighboring `refresh_trigger_cache_async` was doing. Made them
consistent.
The TUI diff filter had a more subtle one. `toggle_diff_filter`
aborts the previous in-flight evaluation on rapid toggles by dropping
the receiver, which makes the *next* `check_diff_filter_results`
tick see `Disconnected` — and unconditionally log "Diff filter:
evaluation failed". Telling the user their action failed when *they*
caused the disconnect is not great. Add a `diff_filter_aborted`
flag, arm it only when there was actually something to abort, and
clear it when consumed. Genuine task panics still get logged loudly.
And finally, `wrkflw run`'s prefilter was calling
`load_trigger_config` directly from an async fn, despite the
library's own docs saying "wrap in spawn_blocking or call from a
blocking context." The watcher and TUI both already do the right
thing here; the CLI was the odd one out. Drifting on this is
*exactly* how the silent-failure holes accumulated in the first
place, so move it onto the blocking pool.
Tests added for each: merge_unique dedup-within-more, the abort-flag
arming and consumption, and a "real disconnect still gets logged"
companion. trigger-filter 73/73, watcher 21/21, ui 21/21,
wrkflw 3/3, clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug another round of silent-failure holes from review
Another review pass on the diff-aware/watch feature, another batch
of "this thing was lying to the user." Same theme as the previous
five rounds, but the failure modes here are louder and harder to
recover from once they bite, so they go in their own commit.
The TUI's `diff_filter_aborted` flag was a footgun. It got armed
when a toggle aborted an in-flight evaluation, but on the OFF path
the next tick observed `rx is None`, returned early, and never
cleared the flag. The *next* toggle ON spawned a fresh task with a
fresh receiver — and inherited the stale flag. So if that fresh
task genuinely failed, the resulting `Disconnected` was treated as
a self-inflicted abort and silently swallowed. Exactly the failure
mode this whole feature has been built to prevent. Clear the flag
at the start of every new evaluation and pin it with a regression
test that drives the full ON → OFF → tick → ON → fail sequence.
The watcher's `is_relevant_event_kind` had a catch-all `Modify(_)
=> false` that dropped `Modify(Any)` events. It turns out kqueue
(FreeBSD, some macOS configurations) and notify's `PollWatcher`
fallback emit `Modify(Any)` for content changes when the underlying
API can't distinguish data from metadata. So the watcher worked
beautifully on Linux inotify and macOS FSEvents and was *completely
silent* on the other backends. Make the match exhaustive on
`ModifyKind` (no catch-all) so future enum additions surface as a
compile error instead of getting routed to "drop", and treat
`Modify(Any)` as relevant.
The watcher's repo-root canonicalization fallback was the worst of
the three. When `canonicalize` failed, the previous code logged a
warning and continued with the raw path. On macOS that means every
notify event arrives as `/private/var/...`, every `strip_prefix`
against the non-canonical `/var/...` rejects, and the user sees
"Watching..." forever with zero workflows ever firing. A watcher
that *looks* alive but never matches a single event is the absolute
worst possible failure mode for this feature. Make it fatal at
startup. If you can't canonicalize the repo root, you don't get a
watcher.
While at it: stamp `--activity-type` through all three integration
points so workflows with `pull_request: { types: [...] }` filters
actually work in the TUI, the watcher, AND the CLI prefilter
(previously only the CLI bothered, and even then only after the
last round of fixes); make `--diff-base` optional on the run
command so the smart auto-detect (origin/HEAD → main → master →
HEAD~1) is actually reachable instead of being shadowed by a
`default_value = "HEAD"` that silently restricted every `--diff`
invocation to uncommitted-only changes; add an info log when
`get_default_diff_base` falls back to HEAD because the working
tree is dirty, since "I have WIP edits, why is `--diff` reporting
nothing triggered?" was a guaranteed support question; extract
`load_trigger_configs` into the trigger-filter crate so the TUI
and watcher stop maintaining near-duplicate parse-and-partition
loops; tighten the activity-type diagnostic to point at the actual
CLI flag instead of a tautology ("requires one"); warn loudly when
a workflow panics inside the watch loop's `catch_unwind` that the
executor may have left containers, tempdirs, or child processes
uncleaned, because a silent panic-recovery on a long-running watch
session is how you end up with `docker ps -a` looking like a
graveyard; and tune `MAX_SETTLE_ROUNDS` from 10 down to 3 so a
sustained `cargo build` can't delay a debounce drain by 5s while
the user wonders if the watcher hung.
Tests cover all three high-priority bugs explicitly. The
aborted-flag test in particular drives the *exact* sequence the
old code got wrong, because the only thing more embarrassing than
a silent-failure hole in a "no more silent failures" feature is
shipping the same failure mode again three weeks later.
* fix(trigger-filter,watcher): plug three silent-failure holes
Review pass turned up three more places where the trigger filter and
watcher would quietly do the wrong thing instead of telling the user
what was going on. Let's fix that.
First: `parse_events` for the `on: [push, pull_request]` sequence
form was silently dropping non-string entries. A typo like
`on: [push, {pull_request: {branches: [main]}}]` collapsed to
`on: [push]` and the pull_request filter *never applied*. The rest
of the parser has been rejecting this kind of nonsense via
`extract_string_list` for a while — the sequence path just never
got the memo. Now it errors with the offending index and kind, same
as everywhere else.
Second: `explain_filter_failure` was building its branch-mismatch
diagnostic from `filter.branches` only, completely ignoring
`branches_ignore`. A workflow with ONLY `branches-ignore: [main]`
running on `main` rendered as `branch 'main' did not match []` —
factually correct, totally useless. Combine both lists with
`!`-prefixed ignore entries so the diagnostic round-trips to the
surface syntax the user actually wrote. Same treatment for tags,
which had the identical bug.
Third: the watcher's per-cycle `collect_workflow_files` rescan was
swallowing errors with `if let Ok(refreshed) = ...` and silently
reusing the previous snapshot forever. Not wrong — the watch loop
*must* continue on transient failures — but silently falling back
to a stale list with zero diagnostic is exactly the silent-skip
pattern the rest of this PR has been plugging. Log at debug level
so a `--verbose` run leaves a trail.
Regression tests for all three. `cargo test` on the two touched
crates: 101 passed, 0 failed.
* fix(trigger-filter,watcher,wrkflw,ui): plug GHA divergence and CLI foot-guns from review
It turns out the path matcher had a "helpful" fallback that tried the
bare filename when a pattern had no slash in it. Neat trick, except
it's not what GitHub Actions does. `*.rs` would match `src/main.rs`
locally and then silently fail to match the same file on GHA, which
is the single worst outcome for a trigger filter whose entire job is
predicting what the real runner will do. Rip it out. Users who want
"any .rs file anywhere" write `**/*.rs` — that's the GHA idiom and
always has been. Pin the new behavior with a test so nobody resurrects
the fallback during a well-meaning refactor.
While at it, a few CLI foot-guns the same review turned up.
`find_repo_root()` shells out to `git rev-parse --show-toplevel`
synchronously and is *not* covered by `GIT_COMMAND_TIMEOUT` — so a
hung git (credential prompt, stuck NFS mount, corrupt repo) would
cheerfully freeze the tokio reactor on `wrkflw run --diff` and
`wrkflw watch`. The TUI already wrapped this call in `spawn_blocking`;
the CLI just didn't. Fix both call sites.
`wrkflw run --diff .gitlab-ci.yml` ran the GitHub-only trigger
prefilter first and then dumped a confusing "Error parsing workflow"
from deep in the YAML parser. Detect GitLab before the prefilter and
bail out with a message that actually tells the user what went wrong.
`max_concurrency` had a floor of 1 but no ceiling, so
\`--max-concurrency 1000000\` was happily accepted. Each concurrent
workflow carries executor state — containers, tempdirs, child
processes — so unbounded values trivially OOM the host with no
helpful signal. Clamp to 256 and warn loudly on the way down.
The watch loop fire-and-forgets the reporter callback via
\`spawn_blocking\` and then drops the JoinHandle. A panicking callback
vanished into tokio's default panic reporter with zero watch-loop
signal, which is exactly the silent-failure mode this whole PR has
been plugging. Supervisor task now awaits the handle and logs any
panic payload via \`wrkflw_logging::error\`. Not great that the
reporter type still invites \`Fn(WatchEvent)\` directly, but at least
a broken reporter is loud now.
Also: the TUI diff-filter toggle used to pay for two git subprocess
calls even when the workflow list was empty — mirror the watcher's
\`configs.is_empty()\` short-circuit so an empty toggle is free. And
dedup the branches/tags formatting in \`explain_filter_failure\`
behind a \`combined_pattern_sources\` helper so a future diagnostic
tweak can't make the two arms drift apart again.
* fix(trigger-filter,watcher): stop git from stealing TTY and fix symlink ignore hole
Another pass over the PR turned up two ugly ones plus a handful of
smaller silent-failure holes that had been sitting there in plain
sight. Let's deal with all of them at once.
The headline issue: every git subprocess we spawn inherited stdin
from the parent TTY and had no `GIT_TERMINAL_PROMPT=0`. In the TUI,
which runs in raw-terminal mode, that means a backgrounded `git diff`
from the diff-filter toggle will happily consume the user's
keystrokes for the full `GIT_COMMAND_TIMEOUT` window (up to 10
seconds) while ratatui sits there wondering where its input went. On
any repo whose remote wants auth it's even worse — git's askpass
races the UI for every keypress. *This is not great.* Wire
`Stdio::null()` and `GIT_TERMINAL_PROMPT=0` into `git_cmd` so every
subprocess is fully insulated from the parent terminal, and do the
same in `find_repo_root` while we're in there.
The other ugly one: `should_ignore_path` compared events against
`repo_root_canonical` only. That happens to work on macOS because
FSEvents canonicalizes everything, but on Linux with a symlinked
working tree (`/home/alice/proj` pointing at `/mnt/work/proj`),
inotify delivers events rooted at the raw `.watch()` argument, so
`strip_prefix` against the canonical form fails for *every single
event* and the entire ignore filter goes silently dark. You end up
eating every `target/`, `node_modules/` and `.git/` write through the
debouncer. Pass both the raw and canonical forms and try each in
turn — the two regression tests pin both the Linux-symlink and the
macOS `/private/var` paths.
While at it:
- `find_repo_root` had no timeout at all. A hung git there leaks a
blocking-pool thread for the rest of the process's life. Replaced
with a hand-rolled `try_wait` poll + kill-on-deadline so we don't
need tokio in this sync path.
- `get_default_diff_base` trusted `git status` stdout without
checking `status.success()`. On a corrupted repo it would cheerfully
claim a clean tree and fall through. Check the exit code like every
other call site does.
- `parse_event_config` silently accepted non-mapping event values, so
\`on: { push: "main" }\` collapsed to "push with no filters" instead
of surfacing the typo. The one remaining silent-drop hole in a
parser that's otherwise militant about these — fixed, with a
regression test.
- `with_max_concurrency(0)` silently clamped to 1 without the warning
its upper-bound sibling emits. Symmetric now.
- Dropped a stale doc comment on `GlobPattern` that still advertised
the bare-filename fallback `path_matcher` explicitly deleted.
All 79 trigger-filter tests and all 25 watcher tests pass, including
the four new regressions (two symlink cases, the parser typo, and the
ignore-file raw/canonical asymmetry).
* fix(trigger-filter,watcher,wrkflw): plug scalability and platform holes from review
Review flagged three issues that would bite real users the moment this
ships: inotify budget exhaustion on monorepos, a per-cycle git
subprocess storm, and Windows `paths:` filters silently never matching.
Plus a handful of smaller cuts.
**inotify pruning.** `watcher.watch(root, Recursive)` expands into one
inotify watch per directory in the tree, so the recursive-on-root call
was happily registering watches on every directory inside `target/`,
`node_modules/`, and `.git/`. On any serious Rust monorepo that blows
past `fs.inotify.max_user_watches = 8192` before the user edits a
single file, at which point notify fails half-open and the watcher
looks alive but never fires. New `setup_watches()` walks the root,
registers each non-ignored child subtree recursively, and leaves the
high-churn subtrees completely un-watched. The ignore filter on the
event hot path stays too, because macOS FSEvents can still deliver
paths the walker didn't register.
**Branch/tag cache.** Every debounced cycle was shelling out to
`git rev-parse --abbrev-ref HEAD` and `git describe --exact-match
HEAD` — for information that changes on `git checkout` and `git tag`,
i.e. basically never. A 40-file save storm was 80 git spawns for no
reason. New `cached_git_state()` on `WorkflowWatcher` with a 3-second
TTL. Not cache-invalidation-on-`.git/HEAD`-event because that would
require whitelisting `.git/` back past the ignore filter and the TTL
does the same job with a quarter of the complexity.
**Windows path separators.** `canonicalize_changed_paths` handed
backslash-separated strings straight to `path_matcher`, and
`glob::Pattern` with `require_literal_separator: true` expects `/`.
So `paths: ['src/**']` silently didn't match `src\main.rs` and every
Windows user would have seen "0 triggered" with no explanation. New
`normalize_separators()` helper runs after `strip_prefix`.
While at it: `WatcherConfig::with_extra_ignore_dirs()` so users can
add `.terraform`, `coverage`, `.next`, `bazel-bin`, etc. without
waiting for us to land each one as a default. `display_workflow_path()`
renders TRIGGERED/SKIPPED labels as repo-relative instead of the
noisy absolute form. `find_repo_root_detailed()` returns a
classified error (`GitNotInstalled` / `Timeout` / `NotInRepository` /
`Other`) so `wrkflw watch` stops telling users "not inside a git
repository" when the real problem was that git wasn't on PATH or the
subprocess hung — that misdiagnosis was sending people down the
wrong fix path every time. Doc comment on `get_changed_files`
spelling out the rename-as-delete+add behavior since `paths-ignore`
users will eventually trip over it. And a pin test for
`workflow_dispatch`/`schedule`/`repository_dispatch` so a future
refactor that starts defaulting `branches:` can't silently break
manual triggers.
Cancellation token and parallel parse are still deferred — both are
larger surface changes and the review called them out as known
follow-ups. Let's not bundle them here.
* fix(trigger-filter,watcher,ui,wrkflw): fix branches+tags OR, kill silent skips from review
The big one first: a push workflow with BOTH `branches:` and `tags:`
set was being evaluated as AND. So if you had
on:
push:
branches: [main]
tags: ['v*']
the filter would never match *anything*. Branch push? tags axis is
empty (no tag on HEAD), reject. Tag push? branches axis is empty
(detached HEAD, no branch), reject. Elegant symmetry. Completely
wrong.
It turns out GitHub Actions treats that case as OR — the workflow
fires if the branch matches `branches:` OR the tag matches `tags:`.
Which is the only thing that makes sense, because a push is on a
branch *or* a tag, never both simultaneously. The previous
sequential-AND check was literally unsatisfiable for the combo case
and no test covered it, so we shipped a feature that silently
skipped an entire class of real-world workflows.
Extract `ref_filters_pass` that handles all four combinations
explicitly and pin it with four regression tests covering branch
push, tag push, miss, and `tags-ignore` exclusion. Update
`explain_filter_failure` to render "neither ref axis matched" for
the combo case so the diagnostic actually mentions both axes
instead of just one.
While at it, plug the rest of the review punch list.
Retire the `find_repo_root` Option wrapper and migrate every call
site (CLI run prefilter, CLI watch, TUI diff-filter) to
`find_repo_root_detailed`. The wrapper collapsed git-not-installed,
timeout, and not-in-repo into one Option::None, which sent users
down the wrong fix path for three of the four failure modes. The
TUI now surfaces the classified error via `DiffFilterOutcome::
Failure` instead of silently showing "0/N would trigger".
Gate the "diff base = HEAD on dirty tree" info log on a `verbose`
parameter threaded through `get_default_diff_base` and
`auto_detect_context_default_base`. The CLI opts in via its
`--verbose` flag; the TUI passes `false` so a hot-path toggle
doesn't flood the log pane on every invocation. This was a
papercut, not a bug, but it was loud.
Move `process::exit` out of `build_event_context` and its helpers.
The function now returns `Result<EventContext, String>` and the
orchestrator owns the exit policy. Helpers that call `exit` from
deep inside are untestable without spawning subprocesses, and we
were accumulating them for no good reason.
Canonicalize `WatcherConfig::workflow_dir` once in `from_config` so
the trigger cache's `HashMap<PathBuf, _>` keys are stable when the
user passes a relative or symlinked workflow directory. Not a bug
in the current CLI (which always passes an absolute path), but the
foot-gun was there for embedders and it was cheap to remove.
Introduce `ShutdownSignal` — a lightweight cancellation handle
backed by `tokio::sync::watch::channel::<bool>`. `WorkflowWatcher::
run` now takes one as a parameter and observes it via
`tokio::select!` inside the idle wait, so triggering cancellation
wakes the loop immediately instead of parking until the next fs
event. The CLI passes `ShutdownSignal::never()` because it still
relies on `process::exit` from Ctrl+C; long-lived hosts (TUI,
future daemon, tests) construct a real signal.
The first implementation of `ShutdownSignal` used `tokio::sync::
Notify` and had a nasty trigger-before-wait race: `notify_waiters()`
only wakes currently-parked tasks, so a caller that triggered
shutdown and then spawned a task that called `wait()` would park
forever. The watch-channel version stores the latest value, so a
subscriber that joins after trigger sees the fired state on the
very next `wait_for` poll. Pinned by
`wait_after_trigger_sees_triggered_state_immediately` so a future
refactor can't silently reintroduce it.
Split the 1884-line `watcher.rs` into five focused submodules with
their own tests: `ignore` (default ignore set + path filter),
`paths` (canonicalization + separator normalization +
display_workflow_path), `event_kind` (notify event classification),
`trigger_cache` (per-cycle parsing cache), and `setup`
(subtree-by-subtree watch registration). Everything is `pub(crate)`
so the public surface is exactly what `lib.rs` re-exports. Nothing
clever, just stop pretending one file is a reasonable home for
1200 lines of hot-path logic.
Add an end-to-end watcher pipeline test
(`end_to_end_pipeline_delivers_watch_event`) that spins up a
tempdir repo, starts the watcher, touches a file in a pre-existing
subtree, and asserts the callback receives a `WatchEvent` with the
changed file. Plus `run_returns_when_shutdown_signal_is_triggered_
before_any_event` to pin the cancellation interrupt path. Both
tests run under `tokio::task::LocalSet` + `spawn_local` because
`run()` is `!Send` (the executor future holds `dyn ContainerRuntime`).
Document the Ctrl+C resource-leak caveat in `wrkflw watch --help`
— a cycle in flight when the user interrupts will get killed by
the OS without running the executor's cleanup, and users who see
orphaned containers deserve to know why.
All tests green (trigger-filter 84, watcher 39, ui 22, wrkflw 3);
clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug the last silent-skip holes and stop sprinkling constants
Another pass over the diff-aware / watch-mode PR after review. The
recurring theme is the same one the previous 24 fix-ups have been
chasing: warn-and-proceed where we should fail loud, plus a pile
of hardcoded constants that drifted across three call sites because
nobody owned them.
It turns out `--event` without `--diff`/`--changed-files`, and
`pull_request` without `--base-branch`, were both still taking the
log-a-warning-and-run path. In a CI script the warning is invisible
and the user sees a session-long stream of "0 triggered" with no
idea why. Same shape in `get_changed_files`: when `ls-files --others`
failed (safe-directory config, transient permission glitch) we
discarded the error in an `if let Ok(_)` block and dropped every
untracked file from the changed set, silently breaking `paths:`
filters for brand-new files. Not great.
Ctrl+C in `watch` was the other big one. We were using
`ShutdownSignal::never()` and relying on the global cleanup
handler's `process::exit(0)` to tear everything down, which meant
a workflow mid-execution got killed by the OS before the executor's
Docker/tempdir cleanup could run. Install a real
`tokio::signal::ctrl_c` handler that triggers the watcher's
shutdown; the global cleanup still runs after `run()` returns, so
we get a clean two-phase teardown instead of an orphaned-container
parade.
While at it, gather every stray constant (`GIT_COMMAND_TIMEOUT`,
`GIT_STATE_CACHE_TTL`, the glob recompile cost, the hardcoded
`"push"` in the TUI) into a single `TriggerFilterConfig` struct and
thread it through the three call sites (CLI prefilter, watcher hot
loop, TUI diff-filter). Having the same five numbers live in three
different places is not my idea of configurability. The config
also owns a process-wide LRU of compiled trigger configs keyed by
`(path, mtime)`, so toggling the TUI filter or re-running a watch
cycle no longer re-parses every YAML and recompiles every glob.
The watcher picks up three other fixes in the same spirit. The git
state cache now keys on `.git/HEAD`'s mtime on top of the TTL, so a
`git checkout` inside the 3s window is caught on the next cycle
instead of evaluating `branches:` filters against the pre-checkout
branch. The debouncer grows bounded (8192 pending paths by default)
and reports per-cycle drops via `WatchEvent.dropped_events` so a
`cargo clean && cargo build` burst can't grow the pending set to
tens of thousands of PathBufs. And the trigger-cache refresh task
now preserves the *previous* cache on panic instead of returning
`HashMap::new()` — a panicking refresh was previously forcing the
next cycle to re-parse every workflow from scratch, which is the
kind of performance cliff you only notice after the bug report.
Parser learned an allowlist of the documented GHA events and warns
(not errors — GHA adds events faster than we update lists) on any
name outside it, so a typo like `pul_request` surfaces once
instead of producing silent "no matching event" diagnostics
forever. `--changed-files` entries now get validated up front:
absolute paths, Windows drive letters, and `..` components get
rejected with a clear error instead of silently failing every
`paths:` glob.
TUI gains a working event selector. The `diff_filter_event` field
was sitting there hardcoded to `"push"` with a comment about
"future UI" — which is a nice way of saying the TUI was silently
lying about which pull_request/workflow_dispatch/schedule workflows
would run. Shift+D now rotates through the six common events and
re-runs the evaluation against the new one. The log buffer also
grew a cap (5000 lines, enforced at every `mark_logs_for_update`
call site) so a long-lived TUI with a lot of diff-filter toggling
doesn't slowly leak memory.
New tests cover every new behavior: LRU hit + mtime invalidation,
`normalize_user_changed_file` rejecting the bad shapes, the unknown
event warning, the debouncer cap counting drops without counting
deduped paths, HEAD-mtime cache invalidation, and the TUI log cap.
Full workspace still green, clippy silent, fmt clean.
Please don't add more file-local constants to this pipeline. Put
them in `TriggerFilterConfig`.
* fix(trigger-filter,watcher): plug parser, git, and worktree silent-skip holes
Another round of review fallout. Six things were wrong, bundled
because they all share the same failure mode: silently producing
"0 triggered" for the wrong reason and sending users chasing a
flag they never needed.
**`on: {}` and `on: []` were accepted at parse time** and then
surfaced as "Workflow does not listen to any events" from the
evaluator — with no workflow path, no YAML context, nothing. GHA
rejects both at upload time. Reject them here too, at parse,
where the error actually carries the file path and the user has
something to grep for.
**`git diff --name-only` and `git ls-files --others` were
newline-terminated.** So a file called `foo\nbar.txt` got
reported as two bogus half-entries, and any non-ASCII filename
came back as `\302\244`-style octal escapes that no `paths:` glob
could ever match. The fix is three characters: `-z`. Rip out the
old `parse_lines` helper and route everything through a new
`parse_nul_separated` that splits on NUL and tolerates the
trailing NUL git sometimes emits. Pinned with a real-git-repo
end-to-end test that commits a file whose name contains a
literal newline and asserts it survives the round trip.
**`head_mtime` stat'd `.git/HEAD` directly**, which returns None
on every linked worktree because in that layout `.git` is a
regular *file* containing `gitdir: /abs/path/to/real/gitdir`.
The watcher's git-state cache then served stale `(branch, tag)`
pairs until the bare TTL expired. Parse the pointer, resolve
relative paths the same way git does (against the `.git` file's
parent), and stat the real HEAD. Plain repos are unchanged.
**The `buffer_unordered` block in watcher.rs had a comment
claiming "Each future wraps a `tokio::spawn`".** It doesn't.
That's actively *load-bearing* — spawning would detach the
futures from the stream and let max_concurrent_executions be
silently ignored. Rewrite the comment to say what the code
actually does and why, so the next refactor doesn't "fix" it
into an unbounded fork bomb. Please don't do that.
**The path-filter diagnostic told users to "pass --diff or
--changed-files"** even when they had *already* passed `--diff`
and it had just come back empty. Confusion ensues. Add
`EventContext::changed_files_explicit`, set it to `true` in
every constructor that actually ran a diff (the three lib.rs
helpers and the watcher's in-tree path), and split the
diagnostic into two cases so "no diff run yet" and "diff was
empty" read differently.
While at it, mark `EventFilter`, `WorkflowTriggerConfig`, and
`TriggerMatchResult` `#[non_exhaustive]`. No external crate
constructs them today — they only read — so this costs nothing
now and buys room to add fields without a major bump later.
`EventContext` stays exhaustive because the watcher builds it
via struct literal and I'm not rewriting that.
Every fix has a pinned test. `cargo test --workspace` is
629/629. `cargo clippy --all-targets` clean. `cargo fmt` clean.
* fix(trigger-filter,watcher,ui,wrkflw): kill dead plumbing and unlock the pattern cache
Staff-level review of the diff-aware watch branch came back with a
pile of findings. Most were easy, but three of them were actually
embarrassing and needed to go before anyone else looked at this.
First, `PATTERN_CACHE` was holding its mutex *across* the blocking
YAML parse + glob compile in `load_trigger_config`. The whole point
of a process-wide LRU is that concurrent callers hitting different
files don't pay for each other's parses — and we were doing the
exact opposite. One slow network-mounted workflow file would stall
every other caller in the process until it finished. Release the
lock on a miss, parse, re-acquire to insert. Racing duplicate
parses on the same file are harmless because late-writer-wins
produces the same value.
Second, `TriggerFilterConfig` shipped two config knobs —
`git_timeout` and `strict_missing_context` — that had builder
setters, had unit tests pinning the builder setters, and were read
by *absolutely nothing* in the library. The timeout lived as a
file-local const in `git.rs` and strict-mode was implemented
entirely in the CLI. Shipping non-functional config knobs is
precisely the kind of drift this crate has been iteratively
patching, so I deleted them rather than half-wiring them. If
anyone ever needs the git timeout override, thread `Duration`
through `run_git` first and add the field at the same commit.
Third, `KNOWN_GHA_EVENTS` contained `pull_request_comment`, which
is not a real GitHub Actions event. Comments on PRs arrive as
`issue_comment` (PRs are issues). Leaving this in the typo-
detection allowlist silently masked the exact warning the function
exists to produce. One-word fix, regression test pinned.
While at it, I cleaned up the other findings:
- Library logging coupling: `collect_unknown_event_warnings`
(renamed from `warn_on_unknown_events`) and
`get_changed_files_with_warnings` no longer dual-emit via
`wrkflw_logging::warning` AND the return value. They collect
warnings as data; hosts own the rendering policy. Warnings now
ride through a new `WorkflowTriggerConfig::warnings` field in
addition to `EventContext::warnings`. The CLI prefilter renders
both. The one remaining library log call is the verbose-gated
`info!` in `get_default_diff_base` — opt-in, informational, not
dual-emission, and fixing it requires signature cascades I
didn't want in this commit.
- `Debouncer::MAX_SETTLE_ROUNDS` was hardcoded to 3. That was
tuned for the default 500ms debounce (3 × 500 = 1.5s worst
case) but a user passing `--debounce 5000` would see a 15s
worst-case drain delay under sustained churn. Replaced with a
wall-clock `MAX_SETTLE_BUDGET = 1500ms` and a dynamic round
cap derived from the debounce window. Preserves the old
behavior at 500ms and caps long windows correctly.
- `head_mtime` was reachable only via `git::head_mtime`, with the
watcher reaching into a sibling crate's `pub` module. Re-export
at the crate root so the coupling is explicit.
- `normalize_user_changed_file` didn't reject embedded NUL bytes.
NUL is not a valid pathname byte on any supported platform, and
letting one through would only produce downstream confusion.
Reject at the boundary with the rest of the input validation.
- `wrkflw watch` had no CLI flag for `extra_ignore_dirs`. The
watcher supported the knob but users couldn't pass extras
without patching source. Added `--ignore-dir`, comma-separated
or repeated.
- `apply_activity_type` was a one-line function called from one
site. Inlined it.
- `cycle_diff_filter_event` re-ran the evaluation by calling
`toggle_diff_filter` twice in sequence — correct but cryptic.
Extracted `rerun_diff_filter_if_active` so the idiom is named
for what it does.
All 332 tests in the workspace still pass, plus the new
regression pins. Clippy clean, fmt clean.
* fix(ui,watcher): plug TUI warning drop + kill double-toggle fragility
Another review pass. The TUI diff filter was *silently* dropping
every `EventContext::warnings` and every `WorkflowTriggerConfig::warnings`
on the floor — exactly the silent-skip failure mode this entire PR
has been fighting for 28 commits. The library deliberately routes
those diagnostics through struct fields instead of logging them
directly, because hosts own the rendering policy. The CLI prefilter
at crates/wrkflw/src/main.rs does the right thing. The TUI just
didn't.
It turns out that 'hosts own the rendering policy' is a contract
the compiler can't enforce. A git ls-files --others safe-directory
rejection, an 'on: pul_request' typo, a corrupted index — none of
it reached the log pane. User saw '0/N would trigger' with no clue
why.
Let's fix that. DiffFilterReport grows a warnings: Vec<String>
field. evaluate_diff_filter now drains context.warnings via
mem::take and harvests each parsed config's .warnings with the
workflow path prefixed, then check_diff_filter_results renders
them under a 'Diff filter: N warning(s)' line, mirroring the CLI
prefilter byte-for-byte. New regression test pins it.
While at it, rerun_diff_filter_if_active was implemented as two
consecutive toggle_diff_filter() calls. Clever. Also fragile: the
whole thing worked only as long as toggle_diff_filter stayed purely
idempotent in opposing directions, and a future side effect
(metrics, tracing, throttle) would have silently broken the rerun
path with zero compile-time signal. Extract spawn_evaluation() and
abort_in_flight_evaluation() as named helpers so the control flow
matches the intent. The existing abort-flag regression tests still
pin the tricky state-machine invariants through the refactor.
And the Debouncer::MAX_SETTLE_BUDGET doc claimed a hard 1.5s cap
on drain latency that the code doesn't actually enforce when the
user passes a debounce window longer than the budget — a single
tokio::time::sleep is not preemptible and the inner sleep runs the
full user-supplied duration. This is not great, but the alternative
(chopping up the inner sleep) would starve the legitimate coalescing
cases. Soften the doc to describe the real worst case instead of
lying about a cap. The pinning tests already cover both short- and
long-window paths.
Minor: help overlay documents 'd' (toggle diff filter) but not
Shift+D (cycle event). Add the missing line.
* fix(trigger-filter,watcher,wrkflw,ui): plug review holes and enforce warning drain
A code review turned up three real correctness bugs and a handful
of latent traps. Rather than patch them piecemeal, fix the whole
batch in one go so the contract that ties them together — "never
silently drop a diagnostic" — is actually load-bearing in the type
system and not just a convention nobody remembers to follow.
The three correctness bugs first:
1. `cached_git_state` was capturing `head_mtime` *after* the two
git reads. The doc comment claimed this was fine because "a
checkout between the read and the store will show up as an
mtime mismatch on the very next call" — it turns out the
comment was wrong. If a checkout lands between the branch/tag
reads, we store the post-checkout mtime alongside the
pre-checkout branch, and the very next lookup's stat matches
the stored value and happily serves the stale branch until
the TTL expires. Move the mtime capture *before* the git
reads so a race produces a visible mismatch. Regression
test interleaves a real `git checkout` between cache calls.
2. `collect_workflow_files_blocking` was returning
`Err(NoWorkflows)` for an empty directory. Watch mode's
entire value proposition is "react to files that don't exist
yet", so refusing to start on an empty `.github/workflows`
is actively wrong. Worse: mid-session, if the user deleted
every workflow file, the rescan failed, the watch loop fell
back to the stale snapshot, and `refresh_trigger_cache_blocking`
retained the deleted entries in the compiled-pattern cache.
So the evaluator kept running against workflows that no
longer existed. Let's fix that. Empty dir is now `Ok(vec![])`,
the watcher logs an info line and carries on, and the
`retain` step correctly evicts stale entries.
3. The pattern cache was keyed on mtime alone. On FAT32 and
some SMB/NFS configs, two edits within the mtime resolution
window (1-2 seconds) collide, and the second parse gets
served a stale compiled config. Combine `(mtime, len)` as
the key — size changes catch the overwhelming majority of
iterative edits, and it's free because we were already
reading the metadata. A content hash would close the
remaining gap but would require a full file read on every
lookup, which is precisely what the cache exists to avoid.
The bigger structural change is the warning-drain contract.
The trigger-filter crate routes non-fatal diagnostics (unknown
event names, `git ls-files --others` failures) through struct
fields instead of calling `wrkflw_logging::warning` directly,
so hosts own the rendering policy. That's the right design.
But it was enforced entirely by convention, and convention is
not a type system.
Proof: `refresh_trigger_cache_blocking` in the watcher was
silently dropping `WorkflowTriggerConfig::warnings` on the
floor. Nobody noticed because the field was a plain
`Vec<String>` and forgetting to iterate it had no consequences.
Exactly the silent-skip mode the rest of this PR exists to
prevent.
Let's fix that too. New `MustDrainWarnings` newtype with a
`debug_assertions`-gated `Drop` impl that `eprintln!`s when a
non-empty buffer is dropped without being observed via `take()`.
Release builds carry zero overhead. Tests and CI catch the
regression immediately. `EventContext::warnings` and
`WorkflowTriggerConfig::warnings` now use it, every host drains
explicitly (`take()`, not read-only iteration), and the watcher
site that was dropping warnings on the floor now routes them
through `wrkflw_logging::warning` at insert time.
One subtlety: the library's LRU cache stashes a clone of the
parsed config alongside the original it returns to the caller.
The clone's warnings would otherwise trip the Drop check on
eviction. Drain them at insert time — the returned original
still carries the full set for the first observer, and
subsequent cache-hit callers see empty warnings, which is the
correct UX (a first-parse diagnostic should not be re-logged
on every cache hit).
While at it, a few opportunistic cleanups the review flagged:
- `run_trigger_prefilter` returns `Result<PrefilterDecision,
String>` instead of calling `std::process::exit` from half
a dozen sites deep inside the body. The flag matrix is now
unit-testable without spawning a subprocess; three new
tests pin the "directory path", "missing path", and
"non-matching skip" branches.
- The `--strict-filter` / `--no-strict-filter` coalescing
was duplicated across the `run` and `watch` handlers. Extract
`effective_strict_filter(strict, no_strict)` so a third host
can't pick up a slightly different version of the same
bool-toggle pattern.
- `normalize_user_changed_file` was accepting whitespace-only
and empty path components (`src/ /foo.rs`, `src//foo.rs`,
trailing slash). These silently fail every `paths:` glob at
evaluation time — exactly the kind of input the function
exists to reject up front. Reject them with a clear "your
flag was wrong" message.
640+ tests passing across the workspace, clippy clean with
`-D warnings`. Nine new tests cover the fixes, each with a
comment naming the prior failure mode so a future refactor
can't silently regress them.
* fix(trigger-filter,watcher,wrkflw): plug the last six review holes
Review pass flagged eight items. Six code fixes, two coverage
gaps — close all of them so the branch can land without carrying
documented regressions.
The biggest one: the `PATTERN_CACHE` docstring promised that the
CLI, the TUI, and the watcher all shared one parse per (path,
mtime) across the process. It turns out they didn't. Each host
handed `load_trigger_config_cached` a different `PathBuf` shape
— raw user input vs relative `read_dir` output vs canonicalized
notify paths — so each host effectively maintained its own
de-facto private cache entry. The cross-host sharing was pure
aspiration.
Fix is to canonicalize inside the cache lookup via
`canonicalize_allowing_missing` (promoted out of the watcher
crate since three consumers need it now) and rewrite
`workflow_path` back to the caller's original form on return so
UI labels still show what the user typed.
Two more loop-discipline oversights in the watcher.
`debouncer.drain().await` sat outside any `tokio::select!`
against the shutdown signal, so Ctrl+C during an active drain
had to wait up to the full debounce window before the loop
observed it. Every other await in `run()` is cancellation-aware
— this one was the last outlier. And the per-cycle callback
supervisor was a detached `tokio::spawn`: correct on the happy
path, but a stuck reporter accumulates one task per cycle
forever because the runtime's own reaper is the only thing
polling the handle. Now held in a `JoinSet`, reaped via
`try_join_next` at the top of each iteration, with a one-shot
warning when the backlog crosses eight.
The rest is smaller. `--max-pending-events` no longer advertises
a misleading `default: 0` in `--help`; the flag is
`Option<usize>` and `Some(0)` is explicitly rejected since a
zero cap would drop every event. The parser gets a doc note
explaining why the duplicate-event-key concern the reviewer
raised is not reachable on `serde_yaml >= 0.9.4` —
`parse_workflow` errors upstream before ever hitting
`parse_events`. Don't remove it; add the HashSet-walk defence
only if someone swaps in a permissive YAML loader.
Two new tests pin the `PATTERN_CACHE` concurrency contract and
the `--diff-head` without `--diff-base` branch in
`build_event_context`. Both were docstring claims that nothing
actually exercised, which is how these things rot in the first
place.
Version bump to 0.8.0 and the README breaking-change section
are deferred to a follow-up PR so the release ceremony isn't
entangled with the code review.
* fix(trigger-filter,watcher,wrkflw): plug four more review holes
Four things the previous round missed, in order of how much they
would have hurt in production.
The callback supervisor JoinSet in the watcher had a warning
threshold but no hard cap. That's cute when the reporter is
*momentarily* slow, but a truly wedged reporter (deadlocked file
sink, stuck network webhook) just accumulates one supervisor per
cycle forever — the warning never reclaims anything, and memory
grows for the life of the session. Add `SUPERVISOR_HARD_CAP = 128`
and drop the current cycle's `WatchEvent` when we hit it. Loud
error, one-shot latch with hysteresis on the reclaim path, the
watch loop keeps running. Not pretty, but the alternative is
eventually having the OOM killer express an opinion.
`parse_nul_separated` was calling `from_utf8_lossy` on every
record, which silently replaced invalid UTF-8 bytes with U+FFFD
and left the file in the change set under a name that could not
match any real glob. This is the exact "silently lying about
which workflows would run" failure mode the rest of the crate
has been fighting, and it was hiding right inside the NUL
parser. Collect the lossy names in a separate list and surface
them via `EventContext::warnings` so the user at least sees
*which* file was dropped from the filter's view.
Strict-filter default-on (`wrkflw run --event push` now exits
1 without `--diff`/`--changed-files`) is a visible CLI behavior
change and the commit message was the only place it was
documented. That's not great. Add a BREAKING_CHANGES.md entry
with migration examples for each escape hatch, exit-code
semantics, and a pointer at `--no-strict-filter` for scripts
that cannot move yet.
While at it, flesh out the `LOG_BUFFER_CAP = 5000` comment in
the TUI (why not 1000, why not 20000, what the two dominant
log sources are), extract the supervisor constants to module
level so they can be reasoned about, and add tests the review
flagged as missing: a compile-time invariant on the supervisor
cap ordering, a runtime test that shutdown still unblocks
`run()` when the reporter callback is wedged (release-gated
blocking thread so cargo can reap it), and three prefilter
tests pinning the strict-mode rejection paths for
`--event`-alone and `pull_request`-without-`--base-branch`.
2026-04-11 20:27:34 +05:30
`wrkflw run` now supports trigger-aware filtering via `--event` , `--diff` ,
and `--changed-files` . When any of those flags is passed, the CLI runs a
prefilter that evaluates the workflow's `on:` block against the simulated
event context before execution — if the triggers don't match, the
workflow is skipped with a clean `exit 0` .
Under the new * * `--strict-filter` default (on)**, running with `--event`
alone — without any way for wrkflw to know the change set — is rejected
with `exit 1` . Previously the CLI proceeded silently with an empty
change set, which meant every workflow gated on `paths:` was reported as
"not triggering" for reasons the user could not see.
Strict mode also rejects simulating `pull_request` / `pull_request_target`
without `--base-branch` , because GitHub Actions evaluates `branches:`
filters on PR events against the target branch, and the same silent-skip
mode applies there.
### Why
`wrkflw run --event push` used to proceed with `changed_files = []` ,
causing every `paths:` -gated workflow to be silently rejected at
evaluation time. Users would then file "why didn't my workflow fire?"
issues for the non-obvious reason that no change set had been supplied.
Strict mode turns that silent failure into a loud, actionable error up
front — it is the default countermeasure for the same class of silent
docs: catch up on everything that landed after v0.7.3 (#107)
v0.7.3 was tagged back in August and then roughly fifty commits
happened. The docs, predictably, noticed none of this.
The README still advertised four TUI tabs when the TUI now has
seven, still listed three runtime modes when there are four, still
declared artifacts/cache/reusable-workflow outputs as "Not
Supported" when all three shipped in #88 and #94, and never
mentioned `wrkflw watch` or the `--event` / `--diff` /
`--changed-files` flags at all. `wrkflw-trigger-filter` and
`wrkflw-watcher` existed in the workspace without READMEs. Two of
the Rust examples referenced a `runtime` field on
`ExecutionConfig` that is actually called `runtime_type`, and
printed a `summary_status` field that doesn't exist. One
`run_wrkflw_tui` example was missing an argument. That kind of
thing.
While at it, BREAKING_CHANGES.md was labeling three entries as
"(v0.7.3)" when the underlying commits all landed *after* the
v0.7.3 tag — so calling them part of that release was, let's say,
a work of fiction. Relabel as "(Unreleased)" with a note up top
pointing at the next release.
New trigger-filter and watcher READMEs are deliberately short —
most users should hit that code through the CLI flags, not by
depending on the crates directly. No point padding them.
Nothing here is a code change. Just the docs finally telling the
truth about what's in the tree.
2026-04-21 23:43:11 +05:30
skip the rest of the trigger-filter work patched iteratively.
feat: add diff-aware selective execution and watch mode (#91)
* feat: add diff-aware selective execution and watch mode
Add two new crates and CLI capabilities that differentiate wrkflw from act:
- `wrkflw-trigger-filter`: Parses workflow `on:` trigger configs (paths,
branches, tags filters) and evaluates them against git diff state to
determine which workflows would actually trigger.
- `wrkflw-watcher`: File system watcher using `notify` crate with
debouncing that auto-detects changes and re-runs affected workflows.
CLI additions:
- `wrkflw run --diff` uses git diff for trigger-aware execution
- `wrkflw run --event <type>` simulates a specific event type
- `wrkflw run --changed-files <list>` manually specifies changed files
- `wrkflw watch` monitors for changes and runs matching workflows
TUI additions:
- Press `d` to toggle diff-aware trigger filter overlay
- Shows green/gray indicators for which workflows would trigger
- Status bar reflects diff filter state
* fix(trigger-filter,watcher): correct glob semantics and fix watcher polling
The glob matchers in both path_matcher and ref_matcher were using
require_literal_separator: false, which means * happily matches /
in file paths and ref names. That's *not* how GitHub Actions works
— * matches anything except /, and ** is what crosses directory
boundaries. So src/* would incorrectly match src/sub/file.rs.
The test that was supposed to catch this (star_does_not_match_slash)
actually tested src/**/*.rs instead. Sneaky.
Set require_literal_separator: true in both matchers and fix the
test to actually assert the correct behavior.
While at it, fix two issues in the watcher's Debouncer:
The mutex handling was inconsistent — add_event and has_pending
silently swallowed poison, but drain called unwrap() and would
panic. Extract a lock_or_recover() helper that consistently
recovers from poison via into_inner() everywhere.
The main watch loop was busy-polling with a 100ms sleep, which is
wasteful and adds needless latency. Replace it with a
tokio::sync::Notify that fires when add_event inserts a path,
so the loop blocks properly until there's actual work to do.
* fix(trigger-filter): correct filter bypass and evaluate activity types
It turns out that when the event context had no branch (e.g., a tag
push), the branch filter check was simply *skipped*, meaning a
workflow with `branches: [main]` would happily match a tag push.
Same problem with the tag filter. The `if let Some(ref branch)`
guard meant "no branch = pass the filter", which is the exact
opposite of what GitHub Actions does.
The fix inverts the logic: check whether the filter *requires*
branches/tags first, and if the context doesn't have one, fail
immediately. While at it, the `types` field was being dutifully
parsed from YAML into `EventFilter.types` and then completely
ignored during evaluation. A workflow with `pull_request: { types:
[opened] }` would trigger on every PR event regardless. That's not
great. Add `activity_type` to `EventContext` and actually evaluate
it.
Also fix the TUI's `toggle_diff_filter` which was running
synchronous git subprocesses on the render thread — blocking the
entire UI while `git diff` thinks about its life choices. Move the
heavy lifting to `std::thread::spawn`. And replace an O(n²)
`Vec::contains` dedup in `get_changed_files` with a HashSet.
* fix(trigger-filter,watcher,ui): fix TUI blocking, watcher event loss, and dual filtering
Three issues found during review, all stemming from the diff-aware
execution feature introduced in the previous commits.
The TUI's toggle_diff_filter() was calling thread::spawn().join(),
which *blocks the main event loop thread* while git commands and
workflow parsing run. If git is slow (large repo, broken index,
network mount), the entire TUI freezes with no way out. Convert to
a fire-and-forget thread with an mpsc channel — the background
thread sends results, and check_diff_filter_results() polls on each
tick via try_recv(). No blocking, no freezing.
The watcher's main loop had a subtle event loss bug. After
notify.notified().await returns and a (potentially long) workflow
execution completes, any events that accumulated during execution
sit in the debouncer. But the loop goes back to notified().await,
which blocks until the *next* filesystem event — the accumulated
ones are orphaned. Add a has_pending() check before awaiting so we
process the backlog immediately.
The executor had an event_filter field on ExecutionConfig, giving it
trigger evaluation responsibilities that don't belong there. The
watcher already filters externally (correctly), creating two
divergent code paths for the same concern. Remove event_filter from
ExecutionConfig entirely and move trigger evaluation to the call
site in main.rs, where it belongs. The executor should just execute.
* fix(trigger-filter,watcher,ui): eliminate duplicated git logic, fix missing untracked files, and parallelize watcher
It turns out the TUI's evaluate_diff_filter was hand-rolling its
own git commands via std::process::Command instead of reusing the
async helpers in trigger-filter::git. Worse, it was only running
`git diff --name-only HEAD` and *not* including untracked files
via `git ls-files --others`. So newly added files were silently
invisible to the TUI diff filter. Not great.
Add synchronous wrappers (get_changed_files_sync,
get_current_branch_sync, get_current_tag_sync) to
trigger-filter::git, sharing the parsing logic with the async
versions via extracted helpers. The TUI now calls these instead
of maintaining its own parallel universe of git invocations.
While at it, fix three more things:
- The watcher was executing triggered workflows sequentially,
meaning a slow workflow would block all others and starve
event processing. Switch to futures::future::join_all for
concurrent execution.
- The debouncer's drain() was unconditionally sleeping for the
full debounce duration even when nothing was pending. Now it
returns immediately when empty and re-checks for new arrivals
after each sleep window so event bursts settle properly.
- The eval reason messages were useless when filters failed due
to missing context (no branch, no tag, no activity type). They
now say *why* the filter couldn't match instead of just dumping
the pattern list.
* fix(watcher,trigger-filter,ui): fix watcher stalls, unbounded concurrency, and sync API duplication
The watcher had a few problems that would make it miserable to use on
any real-world repository.
First, the notify callback was using blocking_send() on the mpsc
channel, which means when the channel fills up (256 slots), the *OS
filesystem event thread* blocks. That's the kind of thing that makes
your entire watcher stop receiving events. Switched to try_send() —
dropping a duplicate event is fine since the debouncer coalesces
anyway.
Second, the watcher was recursively watching the entire repo root
with no exclusions beyond .git/. On any project with a target/ or
node_modules/ directory, this floods the event channel with thousands
of irrelevant build artifact changes. Added a DEFAULT_IGNORE_DIRS
list that filters at the path-component level.
Third, join_all() on the triggered workflow futures meant *unbounded*
concurrent execution. If 20 workflows trigger simultaneously, that's
20 Docker containers spawning at once. Replaced with
buffer_unordered(4) so we don't eat all available resources.
While at it, a few more cleanups from the review:
- Renamed the misleading on_event callback to on_cycle_complete,
since it fires *after* execution completes, not on detection.
- Extracted the 170-line evaluate-and-execute loop from run() into
its own evaluate_and_execute() method.
- Parallelized the two sequential git subprocess calls in
get_changed_files() with tokio::join!.
- Killed the entire sync API in git.rs (three copy-pasted functions).
The TUI's evaluate_diff_filter now spins up a lightweight
current_thread tokio runtime instead of maintaining duplicate sync
wrappers. Having the same logic in two places with two slightly
different implementations is not redundancy, it's a bug farm.
- Renamed LogFilterLevel::to_string() to as_str() because shadowing
the ToString trait with an inherent method is a Clippy lint and
genuinely confusing.
* fix(trigger-filter,watcher,ui): fix debouncer livelock, dead code, and broken diff base
Three issues found during review, all interconnected:
The debouncer's drain() loop had no upper bound on settle rounds.
If filesystem events arrived faster than the debounce window (e.g.
a large build writing hundreds of files), it would spin forever
comparing pending.len() != count_before. Add a MAX_SETTLE_ROUNDS
cap of 10 so it eventually gives up waiting for silence and just
drains what it has.
The trigger-filter crate exported filter_workflows() and
auto_detect_context() — nice reusable helpers — and then both
state.rs and watcher.rs proceeded to ignore them entirely and
inline their own copies of the same logic. Three copies of "build
EventContext, loop over workflows, evaluate triggers" is not
redundancy, it's a maintenance trap. Refactor both callsites to
actually use the helpers they're sitting next to.
The TUI diff filter was hardcoded to diff against HEAD, which only
shows uncommitted changes. Post-commit, the diff is empty and every
workflow gets filtered out. Add get_default_diff_base() which
checks for uncommitted changes first (uses HEAD), then falls back
to merge-base with main/master, then HEAD~1. The TUI now uses this
via auto_detect_context so it shows meaningful results even after
committing.
While at it, cargo fmt touched eval.rs formatting. No logic change.
* fix(trigger-filter): fix staged file blindness, hardcoded default branch, and dead code
get_changed_files() was running `git diff --name-only HEAD` and
proudly calling that "staged + unstaged changes." It turns out
that command only shows *unstaged* changes. Files sitting in the
index after `git add` were completely invisible to trigger
evaluation. Confusion ensues.
Add a parallel `git diff --cached --name-only <base>` call and
merge its results with the unstaged diff, deduplicating paths.
Now we actually see what the user staged. Novel concept.
get_default_diff_base() had "main" and "master" hardcoded as the
only candidates for the merge-base. Repos using "develop", "trunk",
or literally anything else would silently fall back to HEAD~1,
which is almost certainly wrong. Query `git symbolic-ref
refs/remotes/origin/HEAD` first to detect the *actual* default
branch, then fall back to the hardcoded list.
While at it, remove the dead PatternError variant from
TriggerFilterError — it was never constructed anywhere.
* fix(trigger-filter,watcher,ui): consolidate EventContext, fix fallback crash, and harden watch loop
Three call sites — CLI run, TUI toggle, and the watcher — were each
hand-assembling EventContext with subtly different logic. The CLI
forgot to use auto_detect_context, the watcher skipped untracked
files, and the TUI was the only one doing it right. It was only a
matter of time before these diverged further.
Add context_from_diff_range() and context_from_changed_files() to
trigger-filter and use them everywhere. One place to get it right,
zero places to get it wrong.
The get_default_diff_base() fallback was blindly returning "HEAD~1"
which *explodes* on single-commit repos because there is no parent.
Now we verify HEAD~1 resolves first and fall back to the git empty
tree SHA (4b825dc6...) as the ultimate backstop — which means "all
files are changed" rather than "crash and burn."
While at it:
- Wrap on_cycle_complete in spawn_blocking so a blocking callback
can't stall the entire watch loop
- Re-collect workflow files each cycle so newly added .yml files
get picked up without a restart
- Guard rapid 'd' toggle in the TUI against spawning duplicate
background threads by dropping the in-flight receiver first
- Add 15 new tests covering filter_workflows, should_ignore_path,
debouncer livelock prevention, invalid globs, and parser types
* fix(watcher,wrkflw,ui): make watch concurrency configurable and warn on lone --event
Three small things from a code review that were bugging me.
First: the watcher had MAX_CONCURRENT_EXECUTIONS hardcoded to 4.
Fine for a typical .github/workflows directory, terrible for anyone
with 30 workflows who actually wants them to run. Promote it to
DEFAULT_MAX_CONCURRENT_EXECUTIONS, plumb a max_concurrency parameter
through WorkflowWatcher::new, and expose it as --max-concurrency on
the watch command. Clamp to >= 1 because passing 0 to
buffer_unordered would just deadlock, and "deadlock on user input"
is not the user experience I want to ship.
Second: `wrkflw run --event push` (without --diff or
--changed-files) silently builds an EventContext with zero changed
files. Any workflow with a `paths:` filter then "doesn't trigger"
and the user has no idea why. Log a warning explaining exactly what
happened and what to do about it. Not a fix per se — the behavior is
technically correct — but silent surprise is worse than a loud
warning.
While at it: add a comment on the `_watcher` binding in the watch
loop because RecommendedWatcher stops emitting events the moment it
gets dropped, and that's the kind of footgun future-me will
absolutely step on. Also flatten a pointless nested if/else in
status_bar.rs that clippy would have eventually complained about
anyway.
* fix(trigger-filter,watcher,ui): stop lying about which workflows would run
The diff-aware execution feature had a pile of quiet ways to
mislead the user, which is the worst kind of bug in a tool whose
entire purpose is "tell me what would run."
First, invalid glob patterns were silently swallowed. A typo
like `paths: [src/**.rs]` matched nothing and there was no
diagnostic — the workflow just reported "would not trigger" and
everyone assumed it was correct. Compile globs up front in
parse_trigger_config and surface bad patterns as ParseError
pointing at the exact key (e.g. `push.paths`). Pre-compiling
also means we're not re-running Pattern::new on every hot path.
Second, the \`pull_request: branches:\` filter was matched against
the current working branch. GitHub Actions matches it against
the *base* (target) branch, so any workflow guarded by
\`pull_request: branches: [main]\` was reported as "would not
trigger" when evaluated on a feature branch. That's the exact
opposite of what happens on GitHub. Add base_branch to
EventContext, route it through branch_for_filter(), expose a
--base-branch flag on both \`run\` and \`watch\`, and warn loudly
when the user simulates pull_request without it.
Third — and this is the fun one — the TUI's diff filter zipped
background-thread results *positionally* against self.workflows.
The background thread captured workflow_paths at toggle time
and sent back a Vec. If the workflow list reloaded between
toggle and result delivery (a new .yml appears on disk, which
is *exactly* what a file-watcher app does), the positional zip
assigned statuses to the wrong workflows. Send (PathBuf, status)
pairs through the channel and key them by path on receipt.
While at it:
- should_ignore_path matched any component anywhere in the path
against the blacklist, so a file literally named
\`scripts/target\` or \`docs/build\` was silently dropped.
Repo-root-relative + skip-the-leaf fixes it.
- All git commands take an optional cwd and prepend
\`git -C <root>\`, so the watcher queries the repo it's actually
watching instead of whatever the process CWD happens to be.
- Ref names are validated against an allowlist before being
passed to \`git diff\`, so \`--upload-pack=foo\` and friends can't
sneak in through --diff-base.
- For non-HEAD base, the redundant \`git diff --cached <ref>\` was
doing nothing useful — \`git diff <ref>\` already covers staged
+ unstaged. Dropped.
- Workflow files are cached in the watcher and only reparsed
when they actually change. If all of them fail to parse, say
so — before, the user just saw "0 triggered, 0 skipped" and
wondered what they'd broken.
- Empty-tree SHA fallback is now a named constant and logs a
warning when it's used. If you've fallen back to diffing
against the empty tree, you almost certainly don't want to.
- The 200-line Run arm in main.rs is extracted into
run_trigger_prefilter_or_exit().
- Dead TriggerFilterError::IoError variant removed.
Tests: 57 in trigger-filter (incl. PR base_branch semantics,
invalid-glob parse errors, ref validation), 13 in watcher
(incl. file-named-target cases), plus a regression test in the
TUI for the path-keyed mapping.
* fix(watcher,trigger-filter): stop dropping fs events and thrashing globs
A fresh code review turned up a handful of real issues in the
diff-aware execution path. A few of them were silent failure
modes, which is the worst kind.
The watcher had a 256-slot bounded MPSC with `try_send` sitting
between the notify callback and the debouncer. The comment said
dropping was fine "because the debouncer coalesces anyway" — which
is only true for repeat events on the *same* path. Burst 300
distinct file changes at it and ~44 unique paths just vanish, and
any workflow whose `paths:` filter matched exactly one of those
files fails to trigger. Which is the entire point of the feature.
Rip the channel out. The notify callback now pushes straight into
the debouncer's shared set — a hashset insert under a mutex,
bounded in cost, and it dedups for free. No intermediate buffer
to overflow.
It turns out `strip_prefix(&self.repo_root)` was also lying to us.
On macOS, `/var` gets delivered as `/private/var`; symlinked
worktrees have the same problem. Every event would fail the
prefix check, the relative path list ended up empty, and the loop
cheerfully `continue`d without telling anyone. Canonicalize the
repo root once at startup and canonicalize incoming paths before
stripping. Warn in verbose mode when events still don't resolve,
so there's a breadcrumb when it does go wrong.
While in there, the watcher was re-parsing every workflow's
trigger config *and recompiling every glob pattern* on every
cycle, because `filter_workflows` takes `WorkflowDefinition` and
parses on the spot. Add `filter_trigger_configs` taking pre-parsed
`WorkflowTriggerConfig`s, and have the watcher cache them by path,
invalidated only when the backing file actually shows up in
`changed_paths`. Free performance for anyone with more than three
workflow files.
And some smaller stuff the review turned up:
`auto_detect_context` and friends were running three git
subprocesses sequentially for absolutely no reason. `tokio::join!`
them. `validate_ref_name` was rejecting `HEAD@{1}`, `@~1`, and
`origin/main@{upstream}` — all perfectly legitimate git revision
syntax — so extend the allowlist to include `@`, `{`, `}`. The
TUI's diff-filter evaluator was hardcoding `event_name = "push"`;
thread it through as a parameter so future event selection is a
plumbing change, not a rewrite. And `run --diff` crashed with a
confusing "Error parsing workflow" message when pointed at a
directory — catch that up front and point the user at
`wrkflw watch` instead.
* fix(trigger-filter,watcher,ui): plug silent-failure holes and unblock reactor
The review on this branch turned up a handful of "the filter
quietly lies to you" bugs plus a watch loop that cheerfully
calls blocking syscalls from inside an async function. Let's
fix all of it.
First, the *actually* scary one: inline `!`-prefixed patterns
in `branches:`/`tags:`/`paths:` were compiled as literal globs.
The `glob` crate doesn't treat a leading `!` as negation, so
something like `branches: ['main', '!release/old']` would
compile `!release/old` into a pattern that matches the string
`!release/old` and *nothing else*. The exclusion got silently
dropped and the workflow happily triggered on `release/old`.
Route those into the `*_ignore` lists at parse time, support
the `!!literal` escape, and reject mixing inline `!` with a
dedicated `*-ignore` key (GHA itself rejects that). Four new
tests cover it.
Second silent lie: `get_default_diff_base` fell back to the
empty-tree SHA when no better base could be detected, which
made `git diff` list *every tracked file* as changed. The
"filter" then matched everything — the log warning was the
only hint. Make it return an error and let the CLI tell the
user to pass `--diff-base` explicitly. A filter that can't
tell you what it's filtering is worse than no filter.
While at it, wrap every git subprocess in a 10s
`tokio::time::timeout` so a hung git (network fs, credential
prompt, corrupt repo) can't wedge the watch loop forever. And
drop the redundant `git diff --cached HEAD` pass — plain
`git diff HEAD` already covers staged + unstaged + deleted.
It was doing nothing but spawning a second process.
Third problem was the watch loop itself. `collect_workflow_files`
ran `read_dir` synchronously *inside* the async run loop on
every cycle, and the per-cycle canonicalize pass called
`std::fs::canonicalize` on every changed path without ever
hopping onto a blocking thread. On a branch switch touching
thousands of files this stalls the reactor. Move both onto
`spawn_blocking`. Also rewrote `should_ignore_path` to walk
`rel.parent().components()` directly instead of collecting
into a `Vec<Component>` — it runs on every notify event, so
the allocation was genuinely wasteful.
The doc comment on `run()` claimed the callback "will not
stall the async watch loop" because it ran on `spawn_blocking`.
That's misleading — the loop awaits the spawned task, so the
*next* cycle doesn't start until the callback returns. Rewrote
it to describe the actual behavior (events accumulate in the
debouncer during the callback; keep it fast).
While I was in the UI crate, the diff-filter toggle was
spawning a raw OS thread and building a fresh
`current_thread` tokio runtime on it — inside a TUI that's
*already* running on a tokio runtime. This is the kind of
thing that looks fine until you try to debug it. Use
`tokio::task::spawn` on the ambient runtime and push the
workflow-parsing loop onto `spawn_blocking`. Same amount of
concurrency, none of the nested-runtime weirdness.
Also: the TUI hardcodes `push` as the simulated event but
never told the user. A workflow with `on: pull_request` would
show up as SKIPPED and the user had no idea why. Surface it
in the block title as `[DIFF: push]` and mention it in the
toggle log. Extracted to a `const` on `App` so a future event
selector is plumbing-only.
Minor cleanups: consistent `Error:` prefixes in the CLI
prefilter (was mixing "Warning: Failed" with `exit(1)` which
is just confusing), and dropped the now-unused
`wrkflw-logging` dep from `trigger-filter`.
All workspace tests pass, clippy is clean.
* fix(watcher,trigger-filter,ui): plug deleted-file + detached-HEAD holes and kill TUI foot-guns
A principal-engineer review of this branch turned up a pile of
correctness bugs and UX foot-guns that were all individually small
but collectively embarrassing. Let's fix them.
The nastiest one: when a file was deleted, `canonicalize` would fail
on the missing target, the fallback was the raw path from notify, and
on macOS (`/private/var` vs `/var`) or any symlinked working tree the
subsequent `strip_prefix(repo_root)` would drop the event on the
floor. So `rm src/main.rs` would *silently* fail to trigger any
workflow with `paths: [src/**]`. Which — in a tool whose entire point
is "run the workflows for what actually changed" — is not great.
Fixed by walking back to the nearest canonicalizable ancestor and
re-joining the missing tail. It's not pretty, but it's correct, and
it keeps deletions on the happy path.
While at it:
- `should_ignore_path` used to iterate the *absolute* path
components when `strip_prefix` failed, which means a perfectly
innocent `/home/alice/target-acquisition/file.rs` would match the
`target` ignore entry. Now we just refuse to filter anything that
isn't under the watch root. Please don't have directories named
after cargo internals outside your repo, but if you do, we won't
punish you for it.
- `get_current_branch` was returning the literal string `"HEAD"` in
detached-HEAD state, because that's what `git rev-parse
--abbrev-ref HEAD` prints. Any workflow with `branches: [HEAD]`
would then "match" in detached state. Signature is now
`Result<Option<String>, _>`, callers `.ok().flatten()`.
- Notify events are now filtered by kind. Access, metadata, Any,
Other are dropped; only Create, Remove, Modify::Data,
Modify::Name make it to the debouncer. A `git checkout` firing a
thousand rename events would previously blow through
MAX_SETTLE_ROUNDS mid-burst. It still might, but at least we're
not also counting every atime update.
- The run() callback is now fire-and-forget. Previously the watch
loop awaited `spawn_blocking(cb).await`, so a slow reporter (file
sink, Slack webhook, whatever comes next) would stall the next
cycle. Documented as "keep it fast" is not a real defense.
On the TUI side, the diff-filter key (`d`) had three separate
problems.
First, the event was a hardcoded `const DIFF_FILTER_EVENT = "push"`.
Users whose main workflow is `pull_request` would press `d`, see
every single workflow reported as SKIPPED, and file a bug. It's now
a field (`App::diff_filter_event`) so a future event selector is a
data-flow change and not a public API churn.
Second, rapid toggles leaked background tasks. Each press spawned a
fresh git + parse-every-workflow task and just dropped the previous
channel. Now we hold a `JoinHandle` and `abort()` the previous task
before starting a new one.
Third, when `auto_detect_context_default_base` errored (fresh repo,
no default branch, whatever), the UI silently showed every workflow
as None and the summary line said "0/N workflows would trigger" —
with *no* indication that anything had failed. The background task
now returns a `DiffFilterOutcome::{Success, Failure}` and the
failure reason gets pushed into the TUI log.
Also:
- Status bar default hint lost `t trigger` when `d diff` was added.
The `t` keybinding still worked, only discoverability was gone.
Restored.
- Workflows-tab title now reads the dynamic `diff_filter_event`
field instead of the const.
Structural cleanup that was overdue anyway:
- Introduced `WatcherConfig` builder. `WorkflowWatcher::new` already
had eight parameters and a `#[allow(clippy::too_many_arguments)]`
— the next knob was going to push it past ten. The old positional
constructor is kept as a legacy forwarder so the CLI call site
doesn't have to change in the same commit.
- Split `WorkflowWatcher::run` into `refresh_trigger_cache` and
`canonicalize_changed_paths` helpers. The loop was 190 lines of
five concerns inline and completely untestable without spinning
up a real notify watcher.
- New `wrkflw_trigger_filter::load_trigger_config(path)` helper
consolidates read + parse + compile globs. The watcher, the TUI
diff evaluator, and the `run` CLI prefilter now all go through
the same function, so they fail identically on the same broken
file instead of each printing its own flavor of error.
- `parser::resolve_include_and_ignore` replaces 40 lines of near-
identical copy-paste across `branches`/`tags`/`paths`. Having
the same bug in three places is not my idea of redundancy.
- `wrkflw run` prefilter now roots git operations at the repo root
via `find_repo_root()`, matching what the watcher already does.
Consistent behavior when invoked from a subdirectory.
Tests: 12 new tests covering deleted-file canonicalization, event-
kind filtering, detached HEAD, diff-base error path, and the
failure-reporting path in the TUI. Tempfile-backed git tests for
`get_changed_files`, `get_default_diff_base`, and
`get_current_branch`. Regression test for the absolute-path leak in
`should_ignore_path`. 104 tests total across the three crates, all
green, clippy clean.
* fix(watcher,trigger-filter,ui,wrkflw): plug stale cache, silent parse failures, and reactor stalls
- watcher: refresh_trigger_cache compared raw notify paths (absolute,
OS-canonicalized) against read_dir paths (relative), so the
"this workflow file was edited, reparse it" branch was dead code and
watch mode served stale parsed configs forever after the first edit.
Canonicalize both sides into the same form before set membership.
- watcher: refresh_trigger_cache ran sync file I/O on the reactor.
Move it onto a spawn_blocking thread via a new async wrapper; keep
the sync helper as a free function so unit tests don't need a runtime.
- ui: evaluate_diff_filter dropped parse failures via .filter_map(.. .ok()),
leaving users with N workflows showing as "-" and no signal that
their YAML was broken. Capture failures into a new DiffFilterReport
and surface each one in the log pane.
- ui: evaluate_diff_filter ran git helpers with cwd: None, so the TUI
evaluated against whatever directory the user happened to be in
rather than the discovered repo root. Plumb find_repo_root() through.
- trigger-filter: validate_ref_name accepted '..', git's range syntax,
which slipped through into format!("{base}..{head}") interpolation
and produced confusing three-dot errors. Reject it explicitly.
- trigger-filter: move find_repo_root from watcher into trigger-filter
(the right home for our git shell-outs); watcher re-exports it for
source compat.
- wrkflw,watcher: migrate the CLI off the legacy WorkflowWatcher::new
positional ctor to the WatcherConfig builder, then delete the legacy
ctor and the unused filter_workflows API.
* fix(trigger-filter,watcher,wrkflw): plug git zombie + watcher panic + silent filter holes
PR review on the diff-aware/watch branch turned up six issues that
either silently lie to the user or quietly degrade the watch loop
over time. None of them blow up immediately, which is precisely why
they're worth fixing before this lands.
It turns out the 10s `tokio::time::timeout` around every git
subprocess does *nothing* for the failure mode it was built for. A
hung child (NFS stall, credential prompt, corrupt repo) keeps
running after the timeout fires because `tokio::process::Command`
does not enable `kill_on_drop` by default — the future just gets
dropped and the zombie keeps consuming a process slot. On a long-
running watch session against a flaky mount that's exactly how you
end up with hundreds of zombie `git` processes. `kill_on_drop(true)`
on `git_cmd`. One line. Should have been there from the start.
The parser was happy to accept `paths:` + `paths-ignore:` (and the
branches/tags equivalents) on the same event. GitHub Actions rejects
that combination at upload time, so users could iterate locally
against semantics that would later fail in production. Same crate,
`extract_string_list` was silently dropping non-string list entries
via `filter_map(... .as_str())` — a typo like `paths: [{src: foo}]`
yielded an empty list, which then matched everything. The exact
"silently lying about which workflows would run" failure mode the
rest of this branch is built to prevent. Both now surface as
`ParseError` with the offending `event.key[idx]` location.
`auto_detect_context` collapsed a real `git rev-parse` failure to
`branch: None` via `.ok().flatten()`, which made a permission denied
on `.git/HEAD` look identical to a clean detached checkout. The
detached-HEAD path already returns `Ok(None)` from
`get_current_branch`, so the fix is just to drop the `.ok().flatten()`
and let `?` propagate real errors.
The watcher had two ways to silently brick itself. First, when
`context_from_changed_files` errored, it fell back to a context
with `branch: None` and kept going — every `branches:`-filtered
workflow then deterministically rejected for the rest of the session
while the user just saw "0 triggered" forever. Now the cycle short-
circuits, attaches the reason to a new `WatchEvent::error` field,
and the CLI surfaces it. Second, a panic from the executor would
unwind through `buffer_unordered` and kill the entire watch loop —
one rogue workflow takes down the session for everything. I wanted
to use \`tokio::spawn\` for isolation, but the executor's
\`dyn ContainerRuntime\` is not \`Sync\` so the future is not \`Send\`.
\`AssertUnwindSafe(...).catch_unwind()\` from \`futures\` works on the
local future, doesn't require \`Send\`, and gives us the same
"contain it, log it, keep going" behavior.
While at it: cache the canonical workflow path inside each trigger
cache entry so refresh stops re-canonicalizing every workflow on
every cycle (one \`lstat\` walk per file per cycle adds up on a
network mount); drop the no-op \`WorkflowWatcher\` accessor methods
that were pretending to be source-compat shims on a brand-new file;
and split the 130-line, 8-arg \`run_trigger_prefilter_or_exit\` in
main.rs into a \`PrefilterRequest\` struct + two helpers so the
\`#[allow(clippy::too_many_arguments)]\` can go away.
Tests cover all of it: parser combo-rejection (paths/branches/tags),
non-string entry rejection in both \`paths:\` and \`types:\`, the null-
is-empty negative case, canonical-path memoization across two
refresh cycles, and a structural test that
\`AssertUnwindSafe.catch_unwind\` actually classifies a panicking
future as \`Err\` so the watcher's match arm fires. Existing
\`parse_mapping_with_tags\` rewritten to use inline \`!\`-negation
because the old shape now violates the rule it asserts against.
\`cargo test --workspace\`: zero failures. clippy + fmt clean.
* fix(trigger-filter,watcher,ui): kill silent failures and dead code
found in review
A principal-engineer-style review of the diff-aware-execution branch
turned up a small pile of paper cuts. None of them are catastrophic
on their own, but together they're the kind of "everything looks
fine, why is nothing triggering?" failure mode this entire feature
is supposed to *prevent*.
The main offender lives in `extract_glob_list`. It carried a
`compiled.source = display_src;` reassignment with a comment claiming
it preserved the human-visible source for the `!!` escape — except
`pattern_src` and `display_src` are equal in every branch, and
`GlobPattern::new` already wrote `pattern_src` into `compiled.source`
on the line above. So the assignment did nothing, and the comment
lied about it. Collapse the two variables into one `pattern_str`
and delete the dead write. While at it, document the greedy `!!`
behavior properly and pin the `!!!foo` triple-bang case with a
test, since GHA's spec doesn't cover it and we *will* refactor this
function again someday.
The TUI's `toggle_diff_filter` was calling `find_repo_root()`
synchronously on the UI thread. That's a `git rev-parse
--show-toplevel` subprocess. Microseconds on local disk, an
*actual hitch* on a network mount, every single toggle. Move it
into `spawn_blocking` inside the spawned task where it belongs.
`evaluate_and_execute` was also doing two pointless `git`
subprocess calls per cycle when there were zero parseable
workflows. Short-circuit on `configs.is_empty()` and stop paying
for context we'll never use.
While I was in there:
- `canonicalize_changed_paths` used to `filter_map(... .ok())`
away any path it couldn't strip-prefix to the repo root.
Notify *should* never deliver one of those, but if it does,
silently dropping the event is exactly the failure mode this
feature exists to kill. Log the dropped path under `--verbose`.
- `get_default_diff_base` only stripped the `origin/` prefix
from `git symbolic-ref --short` output. If `--short` ever
fails to apply (older git, weird vendor, who knows), the
output is `refs/remotes/origin/main` and the candidate name
ends up wrong. Strip both forms.
- `get_changed_files`'s doc claimed "covers staged + unstaged
+ committed-on-branch." That's *almost* true, but the
modify-then-stage-then-revert-in-WT edge case is genuinely
invisible to `git diff <base>`. Don't promise things we can't
deliver — the trigger filter is an approximation, not a full
index audit.
- `explain_filter_failure` now mentions an empty change set when
a `paths:` filter rejects, so the user who passed `--event
push` without `--diff` gets a directly actionable hint instead
of a `paths: ["src/**"]` dump and a furrowed brow.
- `WorkflowWatcher::run` is an unbounded `loop {}` with no
cancellation path. The CLI fakes graceful shutdown by calling
`process::exit(0)` from a Ctrl+C handler. That works for the
CLI, but anyone embedding this in a daemon or test harness is
going to be unhappy. Document the limitation loudly so the
next person knows it's a known follow-up rather than an
oversight.
- The `JoinHandle::abort` in `toggle_diff_filter` doesn't
actually cancel anything — abort only signals at await
points, and the git futures inside the task may keep running.
Soften the doc to "no longer observed" instead of overstating
the guarantee.
None of this is glamorous. The bias of this entire branch has
been "fail loud, don't lie about what's running," and this just
finishes the cleanup the review surfaced.
Build clean, all 71 trigger-filter / 21 watcher / 19 ui tests
pass, no clippy warnings.
* fix(trigger-filter,watcher,ui,wrkflw): kill five more silent-failure holes from review
A principal-engineer review of the diff-aware/watch branch turned up
five places where this PR was still doing the exact thing the rest of
the branch was built to stop doing: failing quietly. Let's fix them.
`merge_unique` in trigger-filter built a `seen` set from `into` and
then never updated it inside the loop, so duplicates *within* `more`
itself happily slipped through. The function name is a contract — it
just wasn't being honored. Switch to `HashSet::insert`'s "was new"
return so each entry costs one lookup, not two, and add the obvious
regression test.
The watcher's notify callback was the next offender: a stray
`if let Ok(event) = res` swallowed every `notify::Error` — queue
overflow, inotify watch budget exhaustion, kernel disconnects, the
lot. The user gets a cheerful "watching..." banner while the loop is
effectively dead. Log the errors as warnings instead. While at it,
`canonicalize_changed_paths` was doing the same trick with
`unwrap_or_default` on its `JoinError`, mirroring nothing the
neighboring `refresh_trigger_cache_async` was doing. Made them
consistent.
The TUI diff filter had a more subtle one. `toggle_diff_filter`
aborts the previous in-flight evaluation on rapid toggles by dropping
the receiver, which makes the *next* `check_diff_filter_results`
tick see `Disconnected` — and unconditionally log "Diff filter:
evaluation failed". Telling the user their action failed when *they*
caused the disconnect is not great. Add a `diff_filter_aborted`
flag, arm it only when there was actually something to abort, and
clear it when consumed. Genuine task panics still get logged loudly.
And finally, `wrkflw run`'s prefilter was calling
`load_trigger_config` directly from an async fn, despite the
library's own docs saying "wrap in spawn_blocking or call from a
blocking context." The watcher and TUI both already do the right
thing here; the CLI was the odd one out. Drifting on this is
*exactly* how the silent-failure holes accumulated in the first
place, so move it onto the blocking pool.
Tests added for each: merge_unique dedup-within-more, the abort-flag
arming and consumption, and a "real disconnect still gets logged"
companion. trigger-filter 73/73, watcher 21/21, ui 21/21,
wrkflw 3/3, clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug another round of silent-failure holes from review
Another review pass on the diff-aware/watch feature, another batch
of "this thing was lying to the user." Same theme as the previous
five rounds, but the failure modes here are louder and harder to
recover from once they bite, so they go in their own commit.
The TUI's `diff_filter_aborted` flag was a footgun. It got armed
when a toggle aborted an in-flight evaluation, but on the OFF path
the next tick observed `rx is None`, returned early, and never
cleared the flag. The *next* toggle ON spawned a fresh task with a
fresh receiver — and inherited the stale flag. So if that fresh
task genuinely failed, the resulting `Disconnected` was treated as
a self-inflicted abort and silently swallowed. Exactly the failure
mode this whole feature has been built to prevent. Clear the flag
at the start of every new evaluation and pin it with a regression
test that drives the full ON → OFF → tick → ON → fail sequence.
The watcher's `is_relevant_event_kind` had a catch-all `Modify(_)
=> false` that dropped `Modify(Any)` events. It turns out kqueue
(FreeBSD, some macOS configurations) and notify's `PollWatcher`
fallback emit `Modify(Any)` for content changes when the underlying
API can't distinguish data from metadata. So the watcher worked
beautifully on Linux inotify and macOS FSEvents and was *completely
silent* on the other backends. Make the match exhaustive on
`ModifyKind` (no catch-all) so future enum additions surface as a
compile error instead of getting routed to "drop", and treat
`Modify(Any)` as relevant.
The watcher's repo-root canonicalization fallback was the worst of
the three. When `canonicalize` failed, the previous code logged a
warning and continued with the raw path. On macOS that means every
notify event arrives as `/private/var/...`, every `strip_prefix`
against the non-canonical `/var/...` rejects, and the user sees
"Watching..." forever with zero workflows ever firing. A watcher
that *looks* alive but never matches a single event is the absolute
worst possible failure mode for this feature. Make it fatal at
startup. If you can't canonicalize the repo root, you don't get a
watcher.
While at it: stamp `--activity-type` through all three integration
points so workflows with `pull_request: { types: [...] }` filters
actually work in the TUI, the watcher, AND the CLI prefilter
(previously only the CLI bothered, and even then only after the
last round of fixes); make `--diff-base` optional on the run
command so the smart auto-detect (origin/HEAD → main → master →
HEAD~1) is actually reachable instead of being shadowed by a
`default_value = "HEAD"` that silently restricted every `--diff`
invocation to uncommitted-only changes; add an info log when
`get_default_diff_base` falls back to HEAD because the working
tree is dirty, since "I have WIP edits, why is `--diff` reporting
nothing triggered?" was a guaranteed support question; extract
`load_trigger_configs` into the trigger-filter crate so the TUI
and watcher stop maintaining near-duplicate parse-and-partition
loops; tighten the activity-type diagnostic to point at the actual
CLI flag instead of a tautology ("requires one"); warn loudly when
a workflow panics inside the watch loop's `catch_unwind` that the
executor may have left containers, tempdirs, or child processes
uncleaned, because a silent panic-recovery on a long-running watch
session is how you end up with `docker ps -a` looking like a
graveyard; and tune `MAX_SETTLE_ROUNDS` from 10 down to 3 so a
sustained `cargo build` can't delay a debounce drain by 5s while
the user wonders if the watcher hung.
Tests cover all three high-priority bugs explicitly. The
aborted-flag test in particular drives the *exact* sequence the
old code got wrong, because the only thing more embarrassing than
a silent-failure hole in a "no more silent failures" feature is
shipping the same failure mode again three weeks later.
* fix(trigger-filter,watcher): plug three silent-failure holes
Review pass turned up three more places where the trigger filter and
watcher would quietly do the wrong thing instead of telling the user
what was going on. Let's fix that.
First: `parse_events` for the `on: [push, pull_request]` sequence
form was silently dropping non-string entries. A typo like
`on: [push, {pull_request: {branches: [main]}}]` collapsed to
`on: [push]` and the pull_request filter *never applied*. The rest
of the parser has been rejecting this kind of nonsense via
`extract_string_list` for a while — the sequence path just never
got the memo. Now it errors with the offending index and kind, same
as everywhere else.
Second: `explain_filter_failure` was building its branch-mismatch
diagnostic from `filter.branches` only, completely ignoring
`branches_ignore`. A workflow with ONLY `branches-ignore: [main]`
running on `main` rendered as `branch 'main' did not match []` —
factually correct, totally useless. Combine both lists with
`!`-prefixed ignore entries so the diagnostic round-trips to the
surface syntax the user actually wrote. Same treatment for tags,
which had the identical bug.
Third: the watcher's per-cycle `collect_workflow_files` rescan was
swallowing errors with `if let Ok(refreshed) = ...` and silently
reusing the previous snapshot forever. Not wrong — the watch loop
*must* continue on transient failures — but silently falling back
to a stale list with zero diagnostic is exactly the silent-skip
pattern the rest of this PR has been plugging. Log at debug level
so a `--verbose` run leaves a trail.
Regression tests for all three. `cargo test` on the two touched
crates: 101 passed, 0 failed.
* fix(trigger-filter,watcher,wrkflw,ui): plug GHA divergence and CLI foot-guns from review
It turns out the path matcher had a "helpful" fallback that tried the
bare filename when a pattern had no slash in it. Neat trick, except
it's not what GitHub Actions does. `*.rs` would match `src/main.rs`
locally and then silently fail to match the same file on GHA, which
is the single worst outcome for a trigger filter whose entire job is
predicting what the real runner will do. Rip it out. Users who want
"any .rs file anywhere" write `**/*.rs` — that's the GHA idiom and
always has been. Pin the new behavior with a test so nobody resurrects
the fallback during a well-meaning refactor.
While at it, a few CLI foot-guns the same review turned up.
`find_repo_root()` shells out to `git rev-parse --show-toplevel`
synchronously and is *not* covered by `GIT_COMMAND_TIMEOUT` — so a
hung git (credential prompt, stuck NFS mount, corrupt repo) would
cheerfully freeze the tokio reactor on `wrkflw run --diff` and
`wrkflw watch`. The TUI already wrapped this call in `spawn_blocking`;
the CLI just didn't. Fix both call sites.
`wrkflw run --diff .gitlab-ci.yml` ran the GitHub-only trigger
prefilter first and then dumped a confusing "Error parsing workflow"
from deep in the YAML parser. Detect GitLab before the prefilter and
bail out with a message that actually tells the user what went wrong.
`max_concurrency` had a floor of 1 but no ceiling, so
\`--max-concurrency 1000000\` was happily accepted. Each concurrent
workflow carries executor state — containers, tempdirs, child
processes — so unbounded values trivially OOM the host with no
helpful signal. Clamp to 256 and warn loudly on the way down.
The watch loop fire-and-forgets the reporter callback via
\`spawn_blocking\` and then drops the JoinHandle. A panicking callback
vanished into tokio's default panic reporter with zero watch-loop
signal, which is exactly the silent-failure mode this whole PR has
been plugging. Supervisor task now awaits the handle and logs any
panic payload via \`wrkflw_logging::error\`. Not great that the
reporter type still invites \`Fn(WatchEvent)\` directly, but at least
a broken reporter is loud now.
Also: the TUI diff-filter toggle used to pay for two git subprocess
calls even when the workflow list was empty — mirror the watcher's
\`configs.is_empty()\` short-circuit so an empty toggle is free. And
dedup the branches/tags formatting in \`explain_filter_failure\`
behind a \`combined_pattern_sources\` helper so a future diagnostic
tweak can't make the two arms drift apart again.
* fix(trigger-filter,watcher): stop git from stealing TTY and fix symlink ignore hole
Another pass over the PR turned up two ugly ones plus a handful of
smaller silent-failure holes that had been sitting there in plain
sight. Let's deal with all of them at once.
The headline issue: every git subprocess we spawn inherited stdin
from the parent TTY and had no `GIT_TERMINAL_PROMPT=0`. In the TUI,
which runs in raw-terminal mode, that means a backgrounded `git diff`
from the diff-filter toggle will happily consume the user's
keystrokes for the full `GIT_COMMAND_TIMEOUT` window (up to 10
seconds) while ratatui sits there wondering where its input went. On
any repo whose remote wants auth it's even worse — git's askpass
races the UI for every keypress. *This is not great.* Wire
`Stdio::null()` and `GIT_TERMINAL_PROMPT=0` into `git_cmd` so every
subprocess is fully insulated from the parent terminal, and do the
same in `find_repo_root` while we're in there.
The other ugly one: `should_ignore_path` compared events against
`repo_root_canonical` only. That happens to work on macOS because
FSEvents canonicalizes everything, but on Linux with a symlinked
working tree (`/home/alice/proj` pointing at `/mnt/work/proj`),
inotify delivers events rooted at the raw `.watch()` argument, so
`strip_prefix` against the canonical form fails for *every single
event* and the entire ignore filter goes silently dark. You end up
eating every `target/`, `node_modules/` and `.git/` write through the
debouncer. Pass both the raw and canonical forms and try each in
turn — the two regression tests pin both the Linux-symlink and the
macOS `/private/var` paths.
While at it:
- `find_repo_root` had no timeout at all. A hung git there leaks a
blocking-pool thread for the rest of the process's life. Replaced
with a hand-rolled `try_wait` poll + kill-on-deadline so we don't
need tokio in this sync path.
- `get_default_diff_base` trusted `git status` stdout without
checking `status.success()`. On a corrupted repo it would cheerfully
claim a clean tree and fall through. Check the exit code like every
other call site does.
- `parse_event_config` silently accepted non-mapping event values, so
\`on: { push: "main" }\` collapsed to "push with no filters" instead
of surfacing the typo. The one remaining silent-drop hole in a
parser that's otherwise militant about these — fixed, with a
regression test.
- `with_max_concurrency(0)` silently clamped to 1 without the warning
its upper-bound sibling emits. Symmetric now.
- Dropped a stale doc comment on `GlobPattern` that still advertised
the bare-filename fallback `path_matcher` explicitly deleted.
All 79 trigger-filter tests and all 25 watcher tests pass, including
the four new regressions (two symlink cases, the parser typo, and the
ignore-file raw/canonical asymmetry).
* fix(trigger-filter,watcher,wrkflw): plug scalability and platform holes from review
Review flagged three issues that would bite real users the moment this
ships: inotify budget exhaustion on monorepos, a per-cycle git
subprocess storm, and Windows `paths:` filters silently never matching.
Plus a handful of smaller cuts.
**inotify pruning.** `watcher.watch(root, Recursive)` expands into one
inotify watch per directory in the tree, so the recursive-on-root call
was happily registering watches on every directory inside `target/`,
`node_modules/`, and `.git/`. On any serious Rust monorepo that blows
past `fs.inotify.max_user_watches = 8192` before the user edits a
single file, at which point notify fails half-open and the watcher
looks alive but never fires. New `setup_watches()` walks the root,
registers each non-ignored child subtree recursively, and leaves the
high-churn subtrees completely un-watched. The ignore filter on the
event hot path stays too, because macOS FSEvents can still deliver
paths the walker didn't register.
**Branch/tag cache.** Every debounced cycle was shelling out to
`git rev-parse --abbrev-ref HEAD` and `git describe --exact-match
HEAD` — for information that changes on `git checkout` and `git tag`,
i.e. basically never. A 40-file save storm was 80 git spawns for no
reason. New `cached_git_state()` on `WorkflowWatcher` with a 3-second
TTL. Not cache-invalidation-on-`.git/HEAD`-event because that would
require whitelisting `.git/` back past the ignore filter and the TTL
does the same job with a quarter of the complexity.
**Windows path separators.** `canonicalize_changed_paths` handed
backslash-separated strings straight to `path_matcher`, and
`glob::Pattern` with `require_literal_separator: true` expects `/`.
So `paths: ['src/**']` silently didn't match `src\main.rs` and every
Windows user would have seen "0 triggered" with no explanation. New
`normalize_separators()` helper runs after `strip_prefix`.
While at it: `WatcherConfig::with_extra_ignore_dirs()` so users can
add `.terraform`, `coverage`, `.next`, `bazel-bin`, etc. without
waiting for us to land each one as a default. `display_workflow_path()`
renders TRIGGERED/SKIPPED labels as repo-relative instead of the
noisy absolute form. `find_repo_root_detailed()` returns a
classified error (`GitNotInstalled` / `Timeout` / `NotInRepository` /
`Other`) so `wrkflw watch` stops telling users "not inside a git
repository" when the real problem was that git wasn't on PATH or the
subprocess hung — that misdiagnosis was sending people down the
wrong fix path every time. Doc comment on `get_changed_files`
spelling out the rename-as-delete+add behavior since `paths-ignore`
users will eventually trip over it. And a pin test for
`workflow_dispatch`/`schedule`/`repository_dispatch` so a future
refactor that starts defaulting `branches:` can't silently break
manual triggers.
Cancellation token and parallel parse are still deferred — both are
larger surface changes and the review called them out as known
follow-ups. Let's not bundle them here.
* fix(trigger-filter,watcher,ui,wrkflw): fix branches+tags OR, kill silent skips from review
The big one first: a push workflow with BOTH `branches:` and `tags:`
set was being evaluated as AND. So if you had
on:
push:
branches: [main]
tags: ['v*']
the filter would never match *anything*. Branch push? tags axis is
empty (no tag on HEAD), reject. Tag push? branches axis is empty
(detached HEAD, no branch), reject. Elegant symmetry. Completely
wrong.
It turns out GitHub Actions treats that case as OR — the workflow
fires if the branch matches `branches:` OR the tag matches `tags:`.
Which is the only thing that makes sense, because a push is on a
branch *or* a tag, never both simultaneously. The previous
sequential-AND check was literally unsatisfiable for the combo case
and no test covered it, so we shipped a feature that silently
skipped an entire class of real-world workflows.
Extract `ref_filters_pass` that handles all four combinations
explicitly and pin it with four regression tests covering branch
push, tag push, miss, and `tags-ignore` exclusion. Update
`explain_filter_failure` to render "neither ref axis matched" for
the combo case so the diagnostic actually mentions both axes
instead of just one.
While at it, plug the rest of the review punch list.
Retire the `find_repo_root` Option wrapper and migrate every call
site (CLI run prefilter, CLI watch, TUI diff-filter) to
`find_repo_root_detailed`. The wrapper collapsed git-not-installed,
timeout, and not-in-repo into one Option::None, which sent users
down the wrong fix path for three of the four failure modes. The
TUI now surfaces the classified error via `DiffFilterOutcome::
Failure` instead of silently showing "0/N would trigger".
Gate the "diff base = HEAD on dirty tree" info log on a `verbose`
parameter threaded through `get_default_diff_base` and
`auto_detect_context_default_base`. The CLI opts in via its
`--verbose` flag; the TUI passes `false` so a hot-path toggle
doesn't flood the log pane on every invocation. This was a
papercut, not a bug, but it was loud.
Move `process::exit` out of `build_event_context` and its helpers.
The function now returns `Result<EventContext, String>` and the
orchestrator owns the exit policy. Helpers that call `exit` from
deep inside are untestable without spawning subprocesses, and we
were accumulating them for no good reason.
Canonicalize `WatcherConfig::workflow_dir` once in `from_config` so
the trigger cache's `HashMap<PathBuf, _>` keys are stable when the
user passes a relative or symlinked workflow directory. Not a bug
in the current CLI (which always passes an absolute path), but the
foot-gun was there for embedders and it was cheap to remove.
Introduce `ShutdownSignal` — a lightweight cancellation handle
backed by `tokio::sync::watch::channel::<bool>`. `WorkflowWatcher::
run` now takes one as a parameter and observes it via
`tokio::select!` inside the idle wait, so triggering cancellation
wakes the loop immediately instead of parking until the next fs
event. The CLI passes `ShutdownSignal::never()` because it still
relies on `process::exit` from Ctrl+C; long-lived hosts (TUI,
future daemon, tests) construct a real signal.
The first implementation of `ShutdownSignal` used `tokio::sync::
Notify` and had a nasty trigger-before-wait race: `notify_waiters()`
only wakes currently-parked tasks, so a caller that triggered
shutdown and then spawned a task that called `wait()` would park
forever. The watch-channel version stores the latest value, so a
subscriber that joins after trigger sees the fired state on the
very next `wait_for` poll. Pinned by
`wait_after_trigger_sees_triggered_state_immediately` so a future
refactor can't silently reintroduce it.
Split the 1884-line `watcher.rs` into five focused submodules with
their own tests: `ignore` (default ignore set + path filter),
`paths` (canonicalization + separator normalization +
display_workflow_path), `event_kind` (notify event classification),
`trigger_cache` (per-cycle parsing cache), and `setup`
(subtree-by-subtree watch registration). Everything is `pub(crate)`
so the public surface is exactly what `lib.rs` re-exports. Nothing
clever, just stop pretending one file is a reasonable home for
1200 lines of hot-path logic.
Add an end-to-end watcher pipeline test
(`end_to_end_pipeline_delivers_watch_event`) that spins up a
tempdir repo, starts the watcher, touches a file in a pre-existing
subtree, and asserts the callback receives a `WatchEvent` with the
changed file. Plus `run_returns_when_shutdown_signal_is_triggered_
before_any_event` to pin the cancellation interrupt path. Both
tests run under `tokio::task::LocalSet` + `spawn_local` because
`run()` is `!Send` (the executor future holds `dyn ContainerRuntime`).
Document the Ctrl+C resource-leak caveat in `wrkflw watch --help`
— a cycle in flight when the user interrupts will get killed by
the OS without running the executor's cleanup, and users who see
orphaned containers deserve to know why.
All tests green (trigger-filter 84, watcher 39, ui 22, wrkflw 3);
clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug the last silent-skip holes and stop sprinkling constants
Another pass over the diff-aware / watch-mode PR after review. The
recurring theme is the same one the previous 24 fix-ups have been
chasing: warn-and-proceed where we should fail loud, plus a pile
of hardcoded constants that drifted across three call sites because
nobody owned them.
It turns out `--event` without `--diff`/`--changed-files`, and
`pull_request` without `--base-branch`, were both still taking the
log-a-warning-and-run path. In a CI script the warning is invisible
and the user sees a session-long stream of "0 triggered" with no
idea why. Same shape in `get_changed_files`: when `ls-files --others`
failed (safe-directory config, transient permission glitch) we
discarded the error in an `if let Ok(_)` block and dropped every
untracked file from the changed set, silently breaking `paths:`
filters for brand-new files. Not great.
Ctrl+C in `watch` was the other big one. We were using
`ShutdownSignal::never()` and relying on the global cleanup
handler's `process::exit(0)` to tear everything down, which meant
a workflow mid-execution got killed by the OS before the executor's
Docker/tempdir cleanup could run. Install a real
`tokio::signal::ctrl_c` handler that triggers the watcher's
shutdown; the global cleanup still runs after `run()` returns, so
we get a clean two-phase teardown instead of an orphaned-container
parade.
While at it, gather every stray constant (`GIT_COMMAND_TIMEOUT`,
`GIT_STATE_CACHE_TTL`, the glob recompile cost, the hardcoded
`"push"` in the TUI) into a single `TriggerFilterConfig` struct and
thread it through the three call sites (CLI prefilter, watcher hot
loop, TUI diff-filter). Having the same five numbers live in three
different places is not my idea of configurability. The config
also owns a process-wide LRU of compiled trigger configs keyed by
`(path, mtime)`, so toggling the TUI filter or re-running a watch
cycle no longer re-parses every YAML and recompiles every glob.
The watcher picks up three other fixes in the same spirit. The git
state cache now keys on `.git/HEAD`'s mtime on top of the TTL, so a
`git checkout` inside the 3s window is caught on the next cycle
instead of evaluating `branches:` filters against the pre-checkout
branch. The debouncer grows bounded (8192 pending paths by default)
and reports per-cycle drops via `WatchEvent.dropped_events` so a
`cargo clean && cargo build` burst can't grow the pending set to
tens of thousands of PathBufs. And the trigger-cache refresh task
now preserves the *previous* cache on panic instead of returning
`HashMap::new()` — a panicking refresh was previously forcing the
next cycle to re-parse every workflow from scratch, which is the
kind of performance cliff you only notice after the bug report.
Parser learned an allowlist of the documented GHA events and warns
(not errors — GHA adds events faster than we update lists) on any
name outside it, so a typo like `pul_request` surfaces once
instead of producing silent "no matching event" diagnostics
forever. `--changed-files` entries now get validated up front:
absolute paths, Windows drive letters, and `..` components get
rejected with a clear error instead of silently failing every
`paths:` glob.
TUI gains a working event selector. The `diff_filter_event` field
was sitting there hardcoded to `"push"` with a comment about
"future UI" — which is a nice way of saying the TUI was silently
lying about which pull_request/workflow_dispatch/schedule workflows
would run. Shift+D now rotates through the six common events and
re-runs the evaluation against the new one. The log buffer also
grew a cap (5000 lines, enforced at every `mark_logs_for_update`
call site) so a long-lived TUI with a lot of diff-filter toggling
doesn't slowly leak memory.
New tests cover every new behavior: LRU hit + mtime invalidation,
`normalize_user_changed_file` rejecting the bad shapes, the unknown
event warning, the debouncer cap counting drops without counting
deduped paths, HEAD-mtime cache invalidation, and the TUI log cap.
Full workspace still green, clippy silent, fmt clean.
Please don't add more file-local constants to this pipeline. Put
them in `TriggerFilterConfig`.
* fix(trigger-filter,watcher): plug parser, git, and worktree silent-skip holes
Another round of review fallout. Six things were wrong, bundled
because they all share the same failure mode: silently producing
"0 triggered" for the wrong reason and sending users chasing a
flag they never needed.
**`on: {}` and `on: []` were accepted at parse time** and then
surfaced as "Workflow does not listen to any events" from the
evaluator — with no workflow path, no YAML context, nothing. GHA
rejects both at upload time. Reject them here too, at parse,
where the error actually carries the file path and the user has
something to grep for.
**`git diff --name-only` and `git ls-files --others` were
newline-terminated.** So a file called `foo\nbar.txt` got
reported as two bogus half-entries, and any non-ASCII filename
came back as `\302\244`-style octal escapes that no `paths:` glob
could ever match. The fix is three characters: `-z`. Rip out the
old `parse_lines` helper and route everything through a new
`parse_nul_separated` that splits on NUL and tolerates the
trailing NUL git sometimes emits. Pinned with a real-git-repo
end-to-end test that commits a file whose name contains a
literal newline and asserts it survives the round trip.
**`head_mtime` stat'd `.git/HEAD` directly**, which returns None
on every linked worktree because in that layout `.git` is a
regular *file* containing `gitdir: /abs/path/to/real/gitdir`.
The watcher's git-state cache then served stale `(branch, tag)`
pairs until the bare TTL expired. Parse the pointer, resolve
relative paths the same way git does (against the `.git` file's
parent), and stat the real HEAD. Plain repos are unchanged.
**The `buffer_unordered` block in watcher.rs had a comment
claiming "Each future wraps a `tokio::spawn`".** It doesn't.
That's actively *load-bearing* — spawning would detach the
futures from the stream and let max_concurrent_executions be
silently ignored. Rewrite the comment to say what the code
actually does and why, so the next refactor doesn't "fix" it
into an unbounded fork bomb. Please don't do that.
**The path-filter diagnostic told users to "pass --diff or
--changed-files"** even when they had *already* passed `--diff`
and it had just come back empty. Confusion ensues. Add
`EventContext::changed_files_explicit`, set it to `true` in
every constructor that actually ran a diff (the three lib.rs
helpers and the watcher's in-tree path), and split the
diagnostic into two cases so "no diff run yet" and "diff was
empty" read differently.
While at it, mark `EventFilter`, `WorkflowTriggerConfig`, and
`TriggerMatchResult` `#[non_exhaustive]`. No external crate
constructs them today — they only read — so this costs nothing
now and buys room to add fields without a major bump later.
`EventContext` stays exhaustive because the watcher builds it
via struct literal and I'm not rewriting that.
Every fix has a pinned test. `cargo test --workspace` is
629/629. `cargo clippy --all-targets` clean. `cargo fmt` clean.
* fix(trigger-filter,watcher,ui,wrkflw): kill dead plumbing and unlock the pattern cache
Staff-level review of the diff-aware watch branch came back with a
pile of findings. Most were easy, but three of them were actually
embarrassing and needed to go before anyone else looked at this.
First, `PATTERN_CACHE` was holding its mutex *across* the blocking
YAML parse + glob compile in `load_trigger_config`. The whole point
of a process-wide LRU is that concurrent callers hitting different
files don't pay for each other's parses — and we were doing the
exact opposite. One slow network-mounted workflow file would stall
every other caller in the process until it finished. Release the
lock on a miss, parse, re-acquire to insert. Racing duplicate
parses on the same file are harmless because late-writer-wins
produces the same value.
Second, `TriggerFilterConfig` shipped two config knobs —
`git_timeout` and `strict_missing_context` — that had builder
setters, had unit tests pinning the builder setters, and were read
by *absolutely nothing* in the library. The timeout lived as a
file-local const in `git.rs` and strict-mode was implemented
entirely in the CLI. Shipping non-functional config knobs is
precisely the kind of drift this crate has been iteratively
patching, so I deleted them rather than half-wiring them. If
anyone ever needs the git timeout override, thread `Duration`
through `run_git` first and add the field at the same commit.
Third, `KNOWN_GHA_EVENTS` contained `pull_request_comment`, which
is not a real GitHub Actions event. Comments on PRs arrive as
`issue_comment` (PRs are issues). Leaving this in the typo-
detection allowlist silently masked the exact warning the function
exists to produce. One-word fix, regression test pinned.
While at it, I cleaned up the other findings:
- Library logging coupling: `collect_unknown_event_warnings`
(renamed from `warn_on_unknown_events`) and
`get_changed_files_with_warnings` no longer dual-emit via
`wrkflw_logging::warning` AND the return value. They collect
warnings as data; hosts own the rendering policy. Warnings now
ride through a new `WorkflowTriggerConfig::warnings` field in
addition to `EventContext::warnings`. The CLI prefilter renders
both. The one remaining library log call is the verbose-gated
`info!` in `get_default_diff_base` — opt-in, informational, not
dual-emission, and fixing it requires signature cascades I
didn't want in this commit.
- `Debouncer::MAX_SETTLE_ROUNDS` was hardcoded to 3. That was
tuned for the default 500ms debounce (3 × 500 = 1.5s worst
case) but a user passing `--debounce 5000` would see a 15s
worst-case drain delay under sustained churn. Replaced with a
wall-clock `MAX_SETTLE_BUDGET = 1500ms` and a dynamic round
cap derived from the debounce window. Preserves the old
behavior at 500ms and caps long windows correctly.
- `head_mtime` was reachable only via `git::head_mtime`, with the
watcher reaching into a sibling crate's `pub` module. Re-export
at the crate root so the coupling is explicit.
- `normalize_user_changed_file` didn't reject embedded NUL bytes.
NUL is not a valid pathname byte on any supported platform, and
letting one through would only produce downstream confusion.
Reject at the boundary with the rest of the input validation.
- `wrkflw watch` had no CLI flag for `extra_ignore_dirs`. The
watcher supported the knob but users couldn't pass extras
without patching source. Added `--ignore-dir`, comma-separated
or repeated.
- `apply_activity_type` was a one-line function called from one
site. Inlined it.
- `cycle_diff_filter_event` re-ran the evaluation by calling
`toggle_diff_filter` twice in sequence — correct but cryptic.
Extracted `rerun_diff_filter_if_active` so the idiom is named
for what it does.
All 332 tests in the workspace still pass, plus the new
regression pins. Clippy clean, fmt clean.
* fix(ui,watcher): plug TUI warning drop + kill double-toggle fragility
Another review pass. The TUI diff filter was *silently* dropping
every `EventContext::warnings` and every `WorkflowTriggerConfig::warnings`
on the floor — exactly the silent-skip failure mode this entire PR
has been fighting for 28 commits. The library deliberately routes
those diagnostics through struct fields instead of logging them
directly, because hosts own the rendering policy. The CLI prefilter
at crates/wrkflw/src/main.rs does the right thing. The TUI just
didn't.
It turns out that 'hosts own the rendering policy' is a contract
the compiler can't enforce. A git ls-files --others safe-directory
rejection, an 'on: pul_request' typo, a corrupted index — none of
it reached the log pane. User saw '0/N would trigger' with no clue
why.
Let's fix that. DiffFilterReport grows a warnings: Vec<String>
field. evaluate_diff_filter now drains context.warnings via
mem::take and harvests each parsed config's .warnings with the
workflow path prefixed, then check_diff_filter_results renders
them under a 'Diff filter: N warning(s)' line, mirroring the CLI
prefilter byte-for-byte. New regression test pins it.
While at it, rerun_diff_filter_if_active was implemented as two
consecutive toggle_diff_filter() calls. Clever. Also fragile: the
whole thing worked only as long as toggle_diff_filter stayed purely
idempotent in opposing directions, and a future side effect
(metrics, tracing, throttle) would have silently broken the rerun
path with zero compile-time signal. Extract spawn_evaluation() and
abort_in_flight_evaluation() as named helpers so the control flow
matches the intent. The existing abort-flag regression tests still
pin the tricky state-machine invariants through the refactor.
And the Debouncer::MAX_SETTLE_BUDGET doc claimed a hard 1.5s cap
on drain latency that the code doesn't actually enforce when the
user passes a debounce window longer than the budget — a single
tokio::time::sleep is not preemptible and the inner sleep runs the
full user-supplied duration. This is not great, but the alternative
(chopping up the inner sleep) would starve the legitimate coalescing
cases. Soften the doc to describe the real worst case instead of
lying about a cap. The pinning tests already cover both short- and
long-window paths.
Minor: help overlay documents 'd' (toggle diff filter) but not
Shift+D (cycle event). Add the missing line.
* fix(trigger-filter,watcher,wrkflw,ui): plug review holes and enforce warning drain
A code review turned up three real correctness bugs and a handful
of latent traps. Rather than patch them piecemeal, fix the whole
batch in one go so the contract that ties them together — "never
silently drop a diagnostic" — is actually load-bearing in the type
system and not just a convention nobody remembers to follow.
The three correctness bugs first:
1. `cached_git_state` was capturing `head_mtime` *after* the two
git reads. The doc comment claimed this was fine because "a
checkout between the read and the store will show up as an
mtime mismatch on the very next call" — it turns out the
comment was wrong. If a checkout lands between the branch/tag
reads, we store the post-checkout mtime alongside the
pre-checkout branch, and the very next lookup's stat matches
the stored value and happily serves the stale branch until
the TTL expires. Move the mtime capture *before* the git
reads so a race produces a visible mismatch. Regression
test interleaves a real `git checkout` between cache calls.
2. `collect_workflow_files_blocking` was returning
`Err(NoWorkflows)` for an empty directory. Watch mode's
entire value proposition is "react to files that don't exist
yet", so refusing to start on an empty `.github/workflows`
is actively wrong. Worse: mid-session, if the user deleted
every workflow file, the rescan failed, the watch loop fell
back to the stale snapshot, and `refresh_trigger_cache_blocking`
retained the deleted entries in the compiled-pattern cache.
So the evaluator kept running against workflows that no
longer existed. Let's fix that. Empty dir is now `Ok(vec![])`,
the watcher logs an info line and carries on, and the
`retain` step correctly evicts stale entries.
3. The pattern cache was keyed on mtime alone. On FAT32 and
some SMB/NFS configs, two edits within the mtime resolution
window (1-2 seconds) collide, and the second parse gets
served a stale compiled config. Combine `(mtime, len)` as
the key — size changes catch the overwhelming majority of
iterative edits, and it's free because we were already
reading the metadata. A content hash would close the
remaining gap but would require a full file read on every
lookup, which is precisely what the cache exists to avoid.
The bigger structural change is the warning-drain contract.
The trigger-filter crate routes non-fatal diagnostics (unknown
event names, `git ls-files --others` failures) through struct
fields instead of calling `wrkflw_logging::warning` directly,
so hosts own the rendering policy. That's the right design.
But it was enforced entirely by convention, and convention is
not a type system.
Proof: `refresh_trigger_cache_blocking` in the watcher was
silently dropping `WorkflowTriggerConfig::warnings` on the
floor. Nobody noticed because the field was a plain
`Vec<String>` and forgetting to iterate it had no consequences.
Exactly the silent-skip mode the rest of this PR exists to
prevent.
Let's fix that too. New `MustDrainWarnings` newtype with a
`debug_assertions`-gated `Drop` impl that `eprintln!`s when a
non-empty buffer is dropped without being observed via `take()`.
Release builds carry zero overhead. Tests and CI catch the
regression immediately. `EventContext::warnings` and
`WorkflowTriggerConfig::warnings` now use it, every host drains
explicitly (`take()`, not read-only iteration), and the watcher
site that was dropping warnings on the floor now routes them
through `wrkflw_logging::warning` at insert time.
One subtlety: the library's LRU cache stashes a clone of the
parsed config alongside the original it returns to the caller.
The clone's warnings would otherwise trip the Drop check on
eviction. Drain them at insert time — the returned original
still carries the full set for the first observer, and
subsequent cache-hit callers see empty warnings, which is the
correct UX (a first-parse diagnostic should not be re-logged
on every cache hit).
While at it, a few opportunistic cleanups the review flagged:
- `run_trigger_prefilter` returns `Result<PrefilterDecision,
String>` instead of calling `std::process::exit` from half
a dozen sites deep inside the body. The flag matrix is now
unit-testable without spawning a subprocess; three new
tests pin the "directory path", "missing path", and
"non-matching skip" branches.
- The `--strict-filter` / `--no-strict-filter` coalescing
was duplicated across the `run` and `watch` handlers. Extract
`effective_strict_filter(strict, no_strict)` so a third host
can't pick up a slightly different version of the same
bool-toggle pattern.
- `normalize_user_changed_file` was accepting whitespace-only
and empty path components (`src/ /foo.rs`, `src//foo.rs`,
trailing slash). These silently fail every `paths:` glob at
evaluation time — exactly the kind of input the function
exists to reject up front. Reject them with a clear "your
flag was wrong" message.
640+ tests passing across the workspace, clippy clean with
`-D warnings`. Nine new tests cover the fixes, each with a
comment naming the prior failure mode so a future refactor
can't silently regress them.
* fix(trigger-filter,watcher,wrkflw): plug the last six review holes
Review pass flagged eight items. Six code fixes, two coverage
gaps — close all of them so the branch can land without carrying
documented regressions.
The biggest one: the `PATTERN_CACHE` docstring promised that the
CLI, the TUI, and the watcher all shared one parse per (path,
mtime) across the process. It turns out they didn't. Each host
handed `load_trigger_config_cached` a different `PathBuf` shape
— raw user input vs relative `read_dir` output vs canonicalized
notify paths — so each host effectively maintained its own
de-facto private cache entry. The cross-host sharing was pure
aspiration.
Fix is to canonicalize inside the cache lookup via
`canonicalize_allowing_missing` (promoted out of the watcher
crate since three consumers need it now) and rewrite
`workflow_path` back to the caller's original form on return so
UI labels still show what the user typed.
Two more loop-discipline oversights in the watcher.
`debouncer.drain().await` sat outside any `tokio::select!`
against the shutdown signal, so Ctrl+C during an active drain
had to wait up to the full debounce window before the loop
observed it. Every other await in `run()` is cancellation-aware
— this one was the last outlier. And the per-cycle callback
supervisor was a detached `tokio::spawn`: correct on the happy
path, but a stuck reporter accumulates one task per cycle
forever because the runtime's own reaper is the only thing
polling the handle. Now held in a `JoinSet`, reaped via
`try_join_next` at the top of each iteration, with a one-shot
warning when the backlog crosses eight.
The rest is smaller. `--max-pending-events` no longer advertises
a misleading `default: 0` in `--help`; the flag is
`Option<usize>` and `Some(0)` is explicitly rejected since a
zero cap would drop every event. The parser gets a doc note
explaining why the duplicate-event-key concern the reviewer
raised is not reachable on `serde_yaml >= 0.9.4` —
`parse_workflow` errors upstream before ever hitting
`parse_events`. Don't remove it; add the HashSet-walk defence
only if someone swaps in a permissive YAML loader.
Two new tests pin the `PATTERN_CACHE` concurrency contract and
the `--diff-head` without `--diff-base` branch in
`build_event_context`. Both were docstring claims that nothing
actually exercised, which is how these things rot in the first
place.
Version bump to 0.8.0 and the README breaking-change section
are deferred to a follow-up PR so the release ceremony isn't
entangled with the code review.
* fix(trigger-filter,watcher,wrkflw): plug four more review holes
Four things the previous round missed, in order of how much they
would have hurt in production.
The callback supervisor JoinSet in the watcher had a warning
threshold but no hard cap. That's cute when the reporter is
*momentarily* slow, but a truly wedged reporter (deadlocked file
sink, stuck network webhook) just accumulates one supervisor per
cycle forever — the warning never reclaims anything, and memory
grows for the life of the session. Add `SUPERVISOR_HARD_CAP = 128`
and drop the current cycle's `WatchEvent` when we hit it. Loud
error, one-shot latch with hysteresis on the reclaim path, the
watch loop keeps running. Not pretty, but the alternative is
eventually having the OOM killer express an opinion.
`parse_nul_separated` was calling `from_utf8_lossy` on every
record, which silently replaced invalid UTF-8 bytes with U+FFFD
and left the file in the change set under a name that could not
match any real glob. This is the exact "silently lying about
which workflows would run" failure mode the rest of the crate
has been fighting, and it was hiding right inside the NUL
parser. Collect the lossy names in a separate list and surface
them via `EventContext::warnings` so the user at least sees
*which* file was dropped from the filter's view.
Strict-filter default-on (`wrkflw run --event push` now exits
1 without `--diff`/`--changed-files`) is a visible CLI behavior
change and the commit message was the only place it was
documented. That's not great. Add a BREAKING_CHANGES.md entry
with migration examples for each escape hatch, exit-code
semantics, and a pointer at `--no-strict-filter` for scripts
that cannot move yet.
While at it, flesh out the `LOG_BUFFER_CAP = 5000` comment in
the TUI (why not 1000, why not 20000, what the two dominant
log sources are), extract the supervisor constants to module
level so they can be reasoned about, and add tests the review
flagged as missing: a compile-time invariant on the supervisor
cap ordering, a runtime test that shutdown still unblocks
`run()` when the reporter callback is wedged (release-gated
blocking thread so cargo can reap it), and three prefilter
tests pinning the strict-mode rejection paths for
`--event`-alone and `pull_request`-without-`--base-branch`.
2026-04-11 20:27:34 +05:30
### Impact
Scripts that invoked `wrkflw run --event <name> <workflow>` without
also passing `--diff` or `--changed-files` will now fail with:
```
Error: --event was supplied without --diff or --changed-files, so no
changed files are known and any workflow with a `paths:` filter would
be silently skipped. Pass --diff to auto-detect from git, --changed-files
to supply them explicitly, or --no-strict-filter to proceed anyway.
```
Scripts that invoked `wrkflw run --event pull_request <workflow>` without
`--base-branch` will similarly fail with a pointer at the missing flag.
`wrkflw watch --event pull_request` behaves the same way: missing
`--base-branch` is rejected under strict mode.
### Migration
Pick the option that matches your intent:
- **CI script that wants to run only workflows the diff would trigger:**
add `--diff` (auto-detect base branch via `origin/HEAD` → `main` →
`master` → `HEAD~1` ) or pin with `--diff-base <ref>` .
```bash
wrkflw run --diff --event push .github/workflows/ci.yml
```
- **CI script that has its own change set (e.g. from `git diff` in a
wrapper):** pass it explicitly.
```bash
wrkflw run --event push --changed-files src/main.rs,Cargo.toml \
.github/workflows/ci.yml
```
- **Simulating a pull request locally:** add `--base-branch` .
```bash
wrkflw run --event pull_request --base-branch main --diff \
.github/workflows/ci.yml
```
- **Legacy warn-and-proceed behavior (not recommended):** opt out with
docs: catch up on everything that landed after v0.7.3 (#107)
v0.7.3 was tagged back in August and then roughly fifty commits
happened. The docs, predictably, noticed none of this.
The README still advertised four TUI tabs when the TUI now has
seven, still listed three runtime modes when there are four, still
declared artifacts/cache/reusable-workflow outputs as "Not
Supported" when all three shipped in #88 and #94, and never
mentioned `wrkflw watch` or the `--event` / `--diff` /
`--changed-files` flags at all. `wrkflw-trigger-filter` and
`wrkflw-watcher` existed in the workspace without READMEs. Two of
the Rust examples referenced a `runtime` field on
`ExecutionConfig` that is actually called `runtime_type`, and
printed a `summary_status` field that doesn't exist. One
`run_wrkflw_tui` example was missing an argument. That kind of
thing.
While at it, BREAKING_CHANGES.md was labeling three entries as
"(v0.7.3)" when the underlying commits all landed *after* the
v0.7.3 tag — so calling them part of that release was, let's say,
a work of fiction. Relabel as "(Unreleased)" with a note up top
pointing at the next release.
New trigger-filter and watcher READMEs are deliberately short —
most users should hit that code through the CLI flags, not by
depending on the crates directly. No point padding them.
Nothing here is a code change. Just the docs finally telling the
truth about what's in the tree.
2026-04-21 23:43:11 +05:30
`--no-strict-filter` . This restores the pre-strict-filter behavior of
logging a warning and running every workflow anyway. Use this only if
your scripts have already adapted to the old silent-skip semantics and
you cannot change them right now.
feat: add diff-aware selective execution and watch mode (#91)
* feat: add diff-aware selective execution and watch mode
Add two new crates and CLI capabilities that differentiate wrkflw from act:
- `wrkflw-trigger-filter`: Parses workflow `on:` trigger configs (paths,
branches, tags filters) and evaluates them against git diff state to
determine which workflows would actually trigger.
- `wrkflw-watcher`: File system watcher using `notify` crate with
debouncing that auto-detects changes and re-runs affected workflows.
CLI additions:
- `wrkflw run --diff` uses git diff for trigger-aware execution
- `wrkflw run --event <type>` simulates a specific event type
- `wrkflw run --changed-files <list>` manually specifies changed files
- `wrkflw watch` monitors for changes and runs matching workflows
TUI additions:
- Press `d` to toggle diff-aware trigger filter overlay
- Shows green/gray indicators for which workflows would trigger
- Status bar reflects diff filter state
* fix(trigger-filter,watcher): correct glob semantics and fix watcher polling
The glob matchers in both path_matcher and ref_matcher were using
require_literal_separator: false, which means * happily matches /
in file paths and ref names. That's *not* how GitHub Actions works
— * matches anything except /, and ** is what crosses directory
boundaries. So src/* would incorrectly match src/sub/file.rs.
The test that was supposed to catch this (star_does_not_match_slash)
actually tested src/**/*.rs instead. Sneaky.
Set require_literal_separator: true in both matchers and fix the
test to actually assert the correct behavior.
While at it, fix two issues in the watcher's Debouncer:
The mutex handling was inconsistent — add_event and has_pending
silently swallowed poison, but drain called unwrap() and would
panic. Extract a lock_or_recover() helper that consistently
recovers from poison via into_inner() everywhere.
The main watch loop was busy-polling with a 100ms sleep, which is
wasteful and adds needless latency. Replace it with a
tokio::sync::Notify that fires when add_event inserts a path,
so the loop blocks properly until there's actual work to do.
* fix(trigger-filter): correct filter bypass and evaluate activity types
It turns out that when the event context had no branch (e.g., a tag
push), the branch filter check was simply *skipped*, meaning a
workflow with `branches: [main]` would happily match a tag push.
Same problem with the tag filter. The `if let Some(ref branch)`
guard meant "no branch = pass the filter", which is the exact
opposite of what GitHub Actions does.
The fix inverts the logic: check whether the filter *requires*
branches/tags first, and if the context doesn't have one, fail
immediately. While at it, the `types` field was being dutifully
parsed from YAML into `EventFilter.types` and then completely
ignored during evaluation. A workflow with `pull_request: { types:
[opened] }` would trigger on every PR event regardless. That's not
great. Add `activity_type` to `EventContext` and actually evaluate
it.
Also fix the TUI's `toggle_diff_filter` which was running
synchronous git subprocesses on the render thread — blocking the
entire UI while `git diff` thinks about its life choices. Move the
heavy lifting to `std::thread::spawn`. And replace an O(n²)
`Vec::contains` dedup in `get_changed_files` with a HashSet.
* fix(trigger-filter,watcher,ui): fix TUI blocking, watcher event loss, and dual filtering
Three issues found during review, all stemming from the diff-aware
execution feature introduced in the previous commits.
The TUI's toggle_diff_filter() was calling thread::spawn().join(),
which *blocks the main event loop thread* while git commands and
workflow parsing run. If git is slow (large repo, broken index,
network mount), the entire TUI freezes with no way out. Convert to
a fire-and-forget thread with an mpsc channel — the background
thread sends results, and check_diff_filter_results() polls on each
tick via try_recv(). No blocking, no freezing.
The watcher's main loop had a subtle event loss bug. After
notify.notified().await returns and a (potentially long) workflow
execution completes, any events that accumulated during execution
sit in the debouncer. But the loop goes back to notified().await,
which blocks until the *next* filesystem event — the accumulated
ones are orphaned. Add a has_pending() check before awaiting so we
process the backlog immediately.
The executor had an event_filter field on ExecutionConfig, giving it
trigger evaluation responsibilities that don't belong there. The
watcher already filters externally (correctly), creating two
divergent code paths for the same concern. Remove event_filter from
ExecutionConfig entirely and move trigger evaluation to the call
site in main.rs, where it belongs. The executor should just execute.
* fix(trigger-filter,watcher,ui): eliminate duplicated git logic, fix missing untracked files, and parallelize watcher
It turns out the TUI's evaluate_diff_filter was hand-rolling its
own git commands via std::process::Command instead of reusing the
async helpers in trigger-filter::git. Worse, it was only running
`git diff --name-only HEAD` and *not* including untracked files
via `git ls-files --others`. So newly added files were silently
invisible to the TUI diff filter. Not great.
Add synchronous wrappers (get_changed_files_sync,
get_current_branch_sync, get_current_tag_sync) to
trigger-filter::git, sharing the parsing logic with the async
versions via extracted helpers. The TUI now calls these instead
of maintaining its own parallel universe of git invocations.
While at it, fix three more things:
- The watcher was executing triggered workflows sequentially,
meaning a slow workflow would block all others and starve
event processing. Switch to futures::future::join_all for
concurrent execution.
- The debouncer's drain() was unconditionally sleeping for the
full debounce duration even when nothing was pending. Now it
returns immediately when empty and re-checks for new arrivals
after each sleep window so event bursts settle properly.
- The eval reason messages were useless when filters failed due
to missing context (no branch, no tag, no activity type). They
now say *why* the filter couldn't match instead of just dumping
the pattern list.
* fix(watcher,trigger-filter,ui): fix watcher stalls, unbounded concurrency, and sync API duplication
The watcher had a few problems that would make it miserable to use on
any real-world repository.
First, the notify callback was using blocking_send() on the mpsc
channel, which means when the channel fills up (256 slots), the *OS
filesystem event thread* blocks. That's the kind of thing that makes
your entire watcher stop receiving events. Switched to try_send() —
dropping a duplicate event is fine since the debouncer coalesces
anyway.
Second, the watcher was recursively watching the entire repo root
with no exclusions beyond .git/. On any project with a target/ or
node_modules/ directory, this floods the event channel with thousands
of irrelevant build artifact changes. Added a DEFAULT_IGNORE_DIRS
list that filters at the path-component level.
Third, join_all() on the triggered workflow futures meant *unbounded*
concurrent execution. If 20 workflows trigger simultaneously, that's
20 Docker containers spawning at once. Replaced with
buffer_unordered(4) so we don't eat all available resources.
While at it, a few more cleanups from the review:
- Renamed the misleading on_event callback to on_cycle_complete,
since it fires *after* execution completes, not on detection.
- Extracted the 170-line evaluate-and-execute loop from run() into
its own evaluate_and_execute() method.
- Parallelized the two sequential git subprocess calls in
get_changed_files() with tokio::join!.
- Killed the entire sync API in git.rs (three copy-pasted functions).
The TUI's evaluate_diff_filter now spins up a lightweight
current_thread tokio runtime instead of maintaining duplicate sync
wrappers. Having the same logic in two places with two slightly
different implementations is not redundancy, it's a bug farm.
- Renamed LogFilterLevel::to_string() to as_str() because shadowing
the ToString trait with an inherent method is a Clippy lint and
genuinely confusing.
* fix(trigger-filter,watcher,ui): fix debouncer livelock, dead code, and broken diff base
Three issues found during review, all interconnected:
The debouncer's drain() loop had no upper bound on settle rounds.
If filesystem events arrived faster than the debounce window (e.g.
a large build writing hundreds of files), it would spin forever
comparing pending.len() != count_before. Add a MAX_SETTLE_ROUNDS
cap of 10 so it eventually gives up waiting for silence and just
drains what it has.
The trigger-filter crate exported filter_workflows() and
auto_detect_context() — nice reusable helpers — and then both
state.rs and watcher.rs proceeded to ignore them entirely and
inline their own copies of the same logic. Three copies of "build
EventContext, loop over workflows, evaluate triggers" is not
redundancy, it's a maintenance trap. Refactor both callsites to
actually use the helpers they're sitting next to.
The TUI diff filter was hardcoded to diff against HEAD, which only
shows uncommitted changes. Post-commit, the diff is empty and every
workflow gets filtered out. Add get_default_diff_base() which
checks for uncommitted changes first (uses HEAD), then falls back
to merge-base with main/master, then HEAD~1. The TUI now uses this
via auto_detect_context so it shows meaningful results even after
committing.
While at it, cargo fmt touched eval.rs formatting. No logic change.
* fix(trigger-filter): fix staged file blindness, hardcoded default branch, and dead code
get_changed_files() was running `git diff --name-only HEAD` and
proudly calling that "staged + unstaged changes." It turns out
that command only shows *unstaged* changes. Files sitting in the
index after `git add` were completely invisible to trigger
evaluation. Confusion ensues.
Add a parallel `git diff --cached --name-only <base>` call and
merge its results with the unstaged diff, deduplicating paths.
Now we actually see what the user staged. Novel concept.
get_default_diff_base() had "main" and "master" hardcoded as the
only candidates for the merge-base. Repos using "develop", "trunk",
or literally anything else would silently fall back to HEAD~1,
which is almost certainly wrong. Query `git symbolic-ref
refs/remotes/origin/HEAD` first to detect the *actual* default
branch, then fall back to the hardcoded list.
While at it, remove the dead PatternError variant from
TriggerFilterError — it was never constructed anywhere.
* fix(trigger-filter,watcher,ui): consolidate EventContext, fix fallback crash, and harden watch loop
Three call sites — CLI run, TUI toggle, and the watcher — were each
hand-assembling EventContext with subtly different logic. The CLI
forgot to use auto_detect_context, the watcher skipped untracked
files, and the TUI was the only one doing it right. It was only a
matter of time before these diverged further.
Add context_from_diff_range() and context_from_changed_files() to
trigger-filter and use them everywhere. One place to get it right,
zero places to get it wrong.
The get_default_diff_base() fallback was blindly returning "HEAD~1"
which *explodes* on single-commit repos because there is no parent.
Now we verify HEAD~1 resolves first and fall back to the git empty
tree SHA (4b825dc6...) as the ultimate backstop — which means "all
files are changed" rather than "crash and burn."
While at it:
- Wrap on_cycle_complete in spawn_blocking so a blocking callback
can't stall the entire watch loop
- Re-collect workflow files each cycle so newly added .yml files
get picked up without a restart
- Guard rapid 'd' toggle in the TUI against spawning duplicate
background threads by dropping the in-flight receiver first
- Add 15 new tests covering filter_workflows, should_ignore_path,
debouncer livelock prevention, invalid globs, and parser types
* fix(watcher,wrkflw,ui): make watch concurrency configurable and warn on lone --event
Three small things from a code review that were bugging me.
First: the watcher had MAX_CONCURRENT_EXECUTIONS hardcoded to 4.
Fine for a typical .github/workflows directory, terrible for anyone
with 30 workflows who actually wants them to run. Promote it to
DEFAULT_MAX_CONCURRENT_EXECUTIONS, plumb a max_concurrency parameter
through WorkflowWatcher::new, and expose it as --max-concurrency on
the watch command. Clamp to >= 1 because passing 0 to
buffer_unordered would just deadlock, and "deadlock on user input"
is not the user experience I want to ship.
Second: `wrkflw run --event push` (without --diff or
--changed-files) silently builds an EventContext with zero changed
files. Any workflow with a `paths:` filter then "doesn't trigger"
and the user has no idea why. Log a warning explaining exactly what
happened and what to do about it. Not a fix per se — the behavior is
technically correct — but silent surprise is worse than a loud
warning.
While at it: add a comment on the `_watcher` binding in the watch
loop because RecommendedWatcher stops emitting events the moment it
gets dropped, and that's the kind of footgun future-me will
absolutely step on. Also flatten a pointless nested if/else in
status_bar.rs that clippy would have eventually complained about
anyway.
* fix(trigger-filter,watcher,ui): stop lying about which workflows would run
The diff-aware execution feature had a pile of quiet ways to
mislead the user, which is the worst kind of bug in a tool whose
entire purpose is "tell me what would run."
First, invalid glob patterns were silently swallowed. A typo
like `paths: [src/**.rs]` matched nothing and there was no
diagnostic — the workflow just reported "would not trigger" and
everyone assumed it was correct. Compile globs up front in
parse_trigger_config and surface bad patterns as ParseError
pointing at the exact key (e.g. `push.paths`). Pre-compiling
also means we're not re-running Pattern::new on every hot path.
Second, the \`pull_request: branches:\` filter was matched against
the current working branch. GitHub Actions matches it against
the *base* (target) branch, so any workflow guarded by
\`pull_request: branches: [main]\` was reported as "would not
trigger" when evaluated on a feature branch. That's the exact
opposite of what happens on GitHub. Add base_branch to
EventContext, route it through branch_for_filter(), expose a
--base-branch flag on both \`run\` and \`watch\`, and warn loudly
when the user simulates pull_request without it.
Third — and this is the fun one — the TUI's diff filter zipped
background-thread results *positionally* against self.workflows.
The background thread captured workflow_paths at toggle time
and sent back a Vec. If the workflow list reloaded between
toggle and result delivery (a new .yml appears on disk, which
is *exactly* what a file-watcher app does), the positional zip
assigned statuses to the wrong workflows. Send (PathBuf, status)
pairs through the channel and key them by path on receipt.
While at it:
- should_ignore_path matched any component anywhere in the path
against the blacklist, so a file literally named
\`scripts/target\` or \`docs/build\` was silently dropped.
Repo-root-relative + skip-the-leaf fixes it.
- All git commands take an optional cwd and prepend
\`git -C <root>\`, so the watcher queries the repo it's actually
watching instead of whatever the process CWD happens to be.
- Ref names are validated against an allowlist before being
passed to \`git diff\`, so \`--upload-pack=foo\` and friends can't
sneak in through --diff-base.
- For non-HEAD base, the redundant \`git diff --cached <ref>\` was
doing nothing useful — \`git diff <ref>\` already covers staged
+ unstaged. Dropped.
- Workflow files are cached in the watcher and only reparsed
when they actually change. If all of them fail to parse, say
so — before, the user just saw "0 triggered, 0 skipped" and
wondered what they'd broken.
- Empty-tree SHA fallback is now a named constant and logs a
warning when it's used. If you've fallen back to diffing
against the empty tree, you almost certainly don't want to.
- The 200-line Run arm in main.rs is extracted into
run_trigger_prefilter_or_exit().
- Dead TriggerFilterError::IoError variant removed.
Tests: 57 in trigger-filter (incl. PR base_branch semantics,
invalid-glob parse errors, ref validation), 13 in watcher
(incl. file-named-target cases), plus a regression test in the
TUI for the path-keyed mapping.
* fix(watcher,trigger-filter): stop dropping fs events and thrashing globs
A fresh code review turned up a handful of real issues in the
diff-aware execution path. A few of them were silent failure
modes, which is the worst kind.
The watcher had a 256-slot bounded MPSC with `try_send` sitting
between the notify callback and the debouncer. The comment said
dropping was fine "because the debouncer coalesces anyway" — which
is only true for repeat events on the *same* path. Burst 300
distinct file changes at it and ~44 unique paths just vanish, and
any workflow whose `paths:` filter matched exactly one of those
files fails to trigger. Which is the entire point of the feature.
Rip the channel out. The notify callback now pushes straight into
the debouncer's shared set — a hashset insert under a mutex,
bounded in cost, and it dedups for free. No intermediate buffer
to overflow.
It turns out `strip_prefix(&self.repo_root)` was also lying to us.
On macOS, `/var` gets delivered as `/private/var`; symlinked
worktrees have the same problem. Every event would fail the
prefix check, the relative path list ended up empty, and the loop
cheerfully `continue`d without telling anyone. Canonicalize the
repo root once at startup and canonicalize incoming paths before
stripping. Warn in verbose mode when events still don't resolve,
so there's a breadcrumb when it does go wrong.
While in there, the watcher was re-parsing every workflow's
trigger config *and recompiling every glob pattern* on every
cycle, because `filter_workflows` takes `WorkflowDefinition` and
parses on the spot. Add `filter_trigger_configs` taking pre-parsed
`WorkflowTriggerConfig`s, and have the watcher cache them by path,
invalidated only when the backing file actually shows up in
`changed_paths`. Free performance for anyone with more than three
workflow files.
And some smaller stuff the review turned up:
`auto_detect_context` and friends were running three git
subprocesses sequentially for absolutely no reason. `tokio::join!`
them. `validate_ref_name` was rejecting `HEAD@{1}`, `@~1`, and
`origin/main@{upstream}` — all perfectly legitimate git revision
syntax — so extend the allowlist to include `@`, `{`, `}`. The
TUI's diff-filter evaluator was hardcoding `event_name = "push"`;
thread it through as a parameter so future event selection is a
plumbing change, not a rewrite. And `run --diff` crashed with a
confusing "Error parsing workflow" message when pointed at a
directory — catch that up front and point the user at
`wrkflw watch` instead.
* fix(trigger-filter,watcher,ui): plug silent-failure holes and unblock reactor
The review on this branch turned up a handful of "the filter
quietly lies to you" bugs plus a watch loop that cheerfully
calls blocking syscalls from inside an async function. Let's
fix all of it.
First, the *actually* scary one: inline `!`-prefixed patterns
in `branches:`/`tags:`/`paths:` were compiled as literal globs.
The `glob` crate doesn't treat a leading `!` as negation, so
something like `branches: ['main', '!release/old']` would
compile `!release/old` into a pattern that matches the string
`!release/old` and *nothing else*. The exclusion got silently
dropped and the workflow happily triggered on `release/old`.
Route those into the `*_ignore` lists at parse time, support
the `!!literal` escape, and reject mixing inline `!` with a
dedicated `*-ignore` key (GHA itself rejects that). Four new
tests cover it.
Second silent lie: `get_default_diff_base` fell back to the
empty-tree SHA when no better base could be detected, which
made `git diff` list *every tracked file* as changed. The
"filter" then matched everything — the log warning was the
only hint. Make it return an error and let the CLI tell the
user to pass `--diff-base` explicitly. A filter that can't
tell you what it's filtering is worse than no filter.
While at it, wrap every git subprocess in a 10s
`tokio::time::timeout` so a hung git (network fs, credential
prompt, corrupt repo) can't wedge the watch loop forever. And
drop the redundant `git diff --cached HEAD` pass — plain
`git diff HEAD` already covers staged + unstaged + deleted.
It was doing nothing but spawning a second process.
Third problem was the watch loop itself. `collect_workflow_files`
ran `read_dir` synchronously *inside* the async run loop on
every cycle, and the per-cycle canonicalize pass called
`std::fs::canonicalize` on every changed path without ever
hopping onto a blocking thread. On a branch switch touching
thousands of files this stalls the reactor. Move both onto
`spawn_blocking`. Also rewrote `should_ignore_path` to walk
`rel.parent().components()` directly instead of collecting
into a `Vec<Component>` — it runs on every notify event, so
the allocation was genuinely wasteful.
The doc comment on `run()` claimed the callback "will not
stall the async watch loop" because it ran on `spawn_blocking`.
That's misleading — the loop awaits the spawned task, so the
*next* cycle doesn't start until the callback returns. Rewrote
it to describe the actual behavior (events accumulate in the
debouncer during the callback; keep it fast).
While I was in the UI crate, the diff-filter toggle was
spawning a raw OS thread and building a fresh
`current_thread` tokio runtime on it — inside a TUI that's
*already* running on a tokio runtime. This is the kind of
thing that looks fine until you try to debug it. Use
`tokio::task::spawn` on the ambient runtime and push the
workflow-parsing loop onto `spawn_blocking`. Same amount of
concurrency, none of the nested-runtime weirdness.
Also: the TUI hardcodes `push` as the simulated event but
never told the user. A workflow with `on: pull_request` would
show up as SKIPPED and the user had no idea why. Surface it
in the block title as `[DIFF: push]` and mention it in the
toggle log. Extracted to a `const` on `App` so a future event
selector is plumbing-only.
Minor cleanups: consistent `Error:` prefixes in the CLI
prefilter (was mixing "Warning: Failed" with `exit(1)` which
is just confusing), and dropped the now-unused
`wrkflw-logging` dep from `trigger-filter`.
All workspace tests pass, clippy is clean.
* fix(watcher,trigger-filter,ui): plug deleted-file + detached-HEAD holes and kill TUI foot-guns
A principal-engineer review of this branch turned up a pile of
correctness bugs and UX foot-guns that were all individually small
but collectively embarrassing. Let's fix them.
The nastiest one: when a file was deleted, `canonicalize` would fail
on the missing target, the fallback was the raw path from notify, and
on macOS (`/private/var` vs `/var`) or any symlinked working tree the
subsequent `strip_prefix(repo_root)` would drop the event on the
floor. So `rm src/main.rs` would *silently* fail to trigger any
workflow with `paths: [src/**]`. Which — in a tool whose entire point
is "run the workflows for what actually changed" — is not great.
Fixed by walking back to the nearest canonicalizable ancestor and
re-joining the missing tail. It's not pretty, but it's correct, and
it keeps deletions on the happy path.
While at it:
- `should_ignore_path` used to iterate the *absolute* path
components when `strip_prefix` failed, which means a perfectly
innocent `/home/alice/target-acquisition/file.rs` would match the
`target` ignore entry. Now we just refuse to filter anything that
isn't under the watch root. Please don't have directories named
after cargo internals outside your repo, but if you do, we won't
punish you for it.
- `get_current_branch` was returning the literal string `"HEAD"` in
detached-HEAD state, because that's what `git rev-parse
--abbrev-ref HEAD` prints. Any workflow with `branches: [HEAD]`
would then "match" in detached state. Signature is now
`Result<Option<String>, _>`, callers `.ok().flatten()`.
- Notify events are now filtered by kind. Access, metadata, Any,
Other are dropped; only Create, Remove, Modify::Data,
Modify::Name make it to the debouncer. A `git checkout` firing a
thousand rename events would previously blow through
MAX_SETTLE_ROUNDS mid-burst. It still might, but at least we're
not also counting every atime update.
- The run() callback is now fire-and-forget. Previously the watch
loop awaited `spawn_blocking(cb).await`, so a slow reporter (file
sink, Slack webhook, whatever comes next) would stall the next
cycle. Documented as "keep it fast" is not a real defense.
On the TUI side, the diff-filter key (`d`) had three separate
problems.
First, the event was a hardcoded `const DIFF_FILTER_EVENT = "push"`.
Users whose main workflow is `pull_request` would press `d`, see
every single workflow reported as SKIPPED, and file a bug. It's now
a field (`App::diff_filter_event`) so a future event selector is a
data-flow change and not a public API churn.
Second, rapid toggles leaked background tasks. Each press spawned a
fresh git + parse-every-workflow task and just dropped the previous
channel. Now we hold a `JoinHandle` and `abort()` the previous task
before starting a new one.
Third, when `auto_detect_context_default_base` errored (fresh repo,
no default branch, whatever), the UI silently showed every workflow
as None and the summary line said "0/N workflows would trigger" —
with *no* indication that anything had failed. The background task
now returns a `DiffFilterOutcome::{Success, Failure}` and the
failure reason gets pushed into the TUI log.
Also:
- Status bar default hint lost `t trigger` when `d diff` was added.
The `t` keybinding still worked, only discoverability was gone.
Restored.
- Workflows-tab title now reads the dynamic `diff_filter_event`
field instead of the const.
Structural cleanup that was overdue anyway:
- Introduced `WatcherConfig` builder. `WorkflowWatcher::new` already
had eight parameters and a `#[allow(clippy::too_many_arguments)]`
— the next knob was going to push it past ten. The old positional
constructor is kept as a legacy forwarder so the CLI call site
doesn't have to change in the same commit.
- Split `WorkflowWatcher::run` into `refresh_trigger_cache` and
`canonicalize_changed_paths` helpers. The loop was 190 lines of
five concerns inline and completely untestable without spinning
up a real notify watcher.
- New `wrkflw_trigger_filter::load_trigger_config(path)` helper
consolidates read + parse + compile globs. The watcher, the TUI
diff evaluator, and the `run` CLI prefilter now all go through
the same function, so they fail identically on the same broken
file instead of each printing its own flavor of error.
- `parser::resolve_include_and_ignore` replaces 40 lines of near-
identical copy-paste across `branches`/`tags`/`paths`. Having
the same bug in three places is not my idea of redundancy.
- `wrkflw run` prefilter now roots git operations at the repo root
via `find_repo_root()`, matching what the watcher already does.
Consistent behavior when invoked from a subdirectory.
Tests: 12 new tests covering deleted-file canonicalization, event-
kind filtering, detached HEAD, diff-base error path, and the
failure-reporting path in the TUI. Tempfile-backed git tests for
`get_changed_files`, `get_default_diff_base`, and
`get_current_branch`. Regression test for the absolute-path leak in
`should_ignore_path`. 104 tests total across the three crates, all
green, clippy clean.
* fix(watcher,trigger-filter,ui,wrkflw): plug stale cache, silent parse failures, and reactor stalls
- watcher: refresh_trigger_cache compared raw notify paths (absolute,
OS-canonicalized) against read_dir paths (relative), so the
"this workflow file was edited, reparse it" branch was dead code and
watch mode served stale parsed configs forever after the first edit.
Canonicalize both sides into the same form before set membership.
- watcher: refresh_trigger_cache ran sync file I/O on the reactor.
Move it onto a spawn_blocking thread via a new async wrapper; keep
the sync helper as a free function so unit tests don't need a runtime.
- ui: evaluate_diff_filter dropped parse failures via .filter_map(.. .ok()),
leaving users with N workflows showing as "-" and no signal that
their YAML was broken. Capture failures into a new DiffFilterReport
and surface each one in the log pane.
- ui: evaluate_diff_filter ran git helpers with cwd: None, so the TUI
evaluated against whatever directory the user happened to be in
rather than the discovered repo root. Plumb find_repo_root() through.
- trigger-filter: validate_ref_name accepted '..', git's range syntax,
which slipped through into format!("{base}..{head}") interpolation
and produced confusing three-dot errors. Reject it explicitly.
- trigger-filter: move find_repo_root from watcher into trigger-filter
(the right home for our git shell-outs); watcher re-exports it for
source compat.
- wrkflw,watcher: migrate the CLI off the legacy WorkflowWatcher::new
positional ctor to the WatcherConfig builder, then delete the legacy
ctor and the unused filter_workflows API.
* fix(trigger-filter,watcher,wrkflw): plug git zombie + watcher panic + silent filter holes
PR review on the diff-aware/watch branch turned up six issues that
either silently lie to the user or quietly degrade the watch loop
over time. None of them blow up immediately, which is precisely why
they're worth fixing before this lands.
It turns out the 10s `tokio::time::timeout` around every git
subprocess does *nothing* for the failure mode it was built for. A
hung child (NFS stall, credential prompt, corrupt repo) keeps
running after the timeout fires because `tokio::process::Command`
does not enable `kill_on_drop` by default — the future just gets
dropped and the zombie keeps consuming a process slot. On a long-
running watch session against a flaky mount that's exactly how you
end up with hundreds of zombie `git` processes. `kill_on_drop(true)`
on `git_cmd`. One line. Should have been there from the start.
The parser was happy to accept `paths:` + `paths-ignore:` (and the
branches/tags equivalents) on the same event. GitHub Actions rejects
that combination at upload time, so users could iterate locally
against semantics that would later fail in production. Same crate,
`extract_string_list` was silently dropping non-string list entries
via `filter_map(... .as_str())` — a typo like `paths: [{src: foo}]`
yielded an empty list, which then matched everything. The exact
"silently lying about which workflows would run" failure mode the
rest of this branch is built to prevent. Both now surface as
`ParseError` with the offending `event.key[idx]` location.
`auto_detect_context` collapsed a real `git rev-parse` failure to
`branch: None` via `.ok().flatten()`, which made a permission denied
on `.git/HEAD` look identical to a clean detached checkout. The
detached-HEAD path already returns `Ok(None)` from
`get_current_branch`, so the fix is just to drop the `.ok().flatten()`
and let `?` propagate real errors.
The watcher had two ways to silently brick itself. First, when
`context_from_changed_files` errored, it fell back to a context
with `branch: None` and kept going — every `branches:`-filtered
workflow then deterministically rejected for the rest of the session
while the user just saw "0 triggered" forever. Now the cycle short-
circuits, attaches the reason to a new `WatchEvent::error` field,
and the CLI surfaces it. Second, a panic from the executor would
unwind through `buffer_unordered` and kill the entire watch loop —
one rogue workflow takes down the session for everything. I wanted
to use \`tokio::spawn\` for isolation, but the executor's
\`dyn ContainerRuntime\` is not \`Sync\` so the future is not \`Send\`.
\`AssertUnwindSafe(...).catch_unwind()\` from \`futures\` works on the
local future, doesn't require \`Send\`, and gives us the same
"contain it, log it, keep going" behavior.
While at it: cache the canonical workflow path inside each trigger
cache entry so refresh stops re-canonicalizing every workflow on
every cycle (one \`lstat\` walk per file per cycle adds up on a
network mount); drop the no-op \`WorkflowWatcher\` accessor methods
that were pretending to be source-compat shims on a brand-new file;
and split the 130-line, 8-arg \`run_trigger_prefilter_or_exit\` in
main.rs into a \`PrefilterRequest\` struct + two helpers so the
\`#[allow(clippy::too_many_arguments)]\` can go away.
Tests cover all of it: parser combo-rejection (paths/branches/tags),
non-string entry rejection in both \`paths:\` and \`types:\`, the null-
is-empty negative case, canonical-path memoization across two
refresh cycles, and a structural test that
\`AssertUnwindSafe.catch_unwind\` actually classifies a panicking
future as \`Err\` so the watcher's match arm fires. Existing
\`parse_mapping_with_tags\` rewritten to use inline \`!\`-negation
because the old shape now violates the rule it asserts against.
\`cargo test --workspace\`: zero failures. clippy + fmt clean.
* fix(trigger-filter,watcher,ui): kill silent failures and dead code
found in review
A principal-engineer-style review of the diff-aware-execution branch
turned up a small pile of paper cuts. None of them are catastrophic
on their own, but together they're the kind of "everything looks
fine, why is nothing triggering?" failure mode this entire feature
is supposed to *prevent*.
The main offender lives in `extract_glob_list`. It carried a
`compiled.source = display_src;` reassignment with a comment claiming
it preserved the human-visible source for the `!!` escape — except
`pattern_src` and `display_src` are equal in every branch, and
`GlobPattern::new` already wrote `pattern_src` into `compiled.source`
on the line above. So the assignment did nothing, and the comment
lied about it. Collapse the two variables into one `pattern_str`
and delete the dead write. While at it, document the greedy `!!`
behavior properly and pin the `!!!foo` triple-bang case with a
test, since GHA's spec doesn't cover it and we *will* refactor this
function again someday.
The TUI's `toggle_diff_filter` was calling `find_repo_root()`
synchronously on the UI thread. That's a `git rev-parse
--show-toplevel` subprocess. Microseconds on local disk, an
*actual hitch* on a network mount, every single toggle. Move it
into `spawn_blocking` inside the spawned task where it belongs.
`evaluate_and_execute` was also doing two pointless `git`
subprocess calls per cycle when there were zero parseable
workflows. Short-circuit on `configs.is_empty()` and stop paying
for context we'll never use.
While I was in there:
- `canonicalize_changed_paths` used to `filter_map(... .ok())`
away any path it couldn't strip-prefix to the repo root.
Notify *should* never deliver one of those, but if it does,
silently dropping the event is exactly the failure mode this
feature exists to kill. Log the dropped path under `--verbose`.
- `get_default_diff_base` only stripped the `origin/` prefix
from `git symbolic-ref --short` output. If `--short` ever
fails to apply (older git, weird vendor, who knows), the
output is `refs/remotes/origin/main` and the candidate name
ends up wrong. Strip both forms.
- `get_changed_files`'s doc claimed "covers staged + unstaged
+ committed-on-branch." That's *almost* true, but the
modify-then-stage-then-revert-in-WT edge case is genuinely
invisible to `git diff <base>`. Don't promise things we can't
deliver — the trigger filter is an approximation, not a full
index audit.
- `explain_filter_failure` now mentions an empty change set when
a `paths:` filter rejects, so the user who passed `--event
push` without `--diff` gets a directly actionable hint instead
of a `paths: ["src/**"]` dump and a furrowed brow.
- `WorkflowWatcher::run` is an unbounded `loop {}` with no
cancellation path. The CLI fakes graceful shutdown by calling
`process::exit(0)` from a Ctrl+C handler. That works for the
CLI, but anyone embedding this in a daemon or test harness is
going to be unhappy. Document the limitation loudly so the
next person knows it's a known follow-up rather than an
oversight.
- The `JoinHandle::abort` in `toggle_diff_filter` doesn't
actually cancel anything — abort only signals at await
points, and the git futures inside the task may keep running.
Soften the doc to "no longer observed" instead of overstating
the guarantee.
None of this is glamorous. The bias of this entire branch has
been "fail loud, don't lie about what's running," and this just
finishes the cleanup the review surfaced.
Build clean, all 71 trigger-filter / 21 watcher / 19 ui tests
pass, no clippy warnings.
* fix(trigger-filter,watcher,ui,wrkflw): kill five more silent-failure holes from review
A principal-engineer review of the diff-aware/watch branch turned up
five places where this PR was still doing the exact thing the rest of
the branch was built to stop doing: failing quietly. Let's fix them.
`merge_unique` in trigger-filter built a `seen` set from `into` and
then never updated it inside the loop, so duplicates *within* `more`
itself happily slipped through. The function name is a contract — it
just wasn't being honored. Switch to `HashSet::insert`'s "was new"
return so each entry costs one lookup, not two, and add the obvious
regression test.
The watcher's notify callback was the next offender: a stray
`if let Ok(event) = res` swallowed every `notify::Error` — queue
overflow, inotify watch budget exhaustion, kernel disconnects, the
lot. The user gets a cheerful "watching..." banner while the loop is
effectively dead. Log the errors as warnings instead. While at it,
`canonicalize_changed_paths` was doing the same trick with
`unwrap_or_default` on its `JoinError`, mirroring nothing the
neighboring `refresh_trigger_cache_async` was doing. Made them
consistent.
The TUI diff filter had a more subtle one. `toggle_diff_filter`
aborts the previous in-flight evaluation on rapid toggles by dropping
the receiver, which makes the *next* `check_diff_filter_results`
tick see `Disconnected` — and unconditionally log "Diff filter:
evaluation failed". Telling the user their action failed when *they*
caused the disconnect is not great. Add a `diff_filter_aborted`
flag, arm it only when there was actually something to abort, and
clear it when consumed. Genuine task panics still get logged loudly.
And finally, `wrkflw run`'s prefilter was calling
`load_trigger_config` directly from an async fn, despite the
library's own docs saying "wrap in spawn_blocking or call from a
blocking context." The watcher and TUI both already do the right
thing here; the CLI was the odd one out. Drifting on this is
*exactly* how the silent-failure holes accumulated in the first
place, so move it onto the blocking pool.
Tests added for each: merge_unique dedup-within-more, the abort-flag
arming and consumption, and a "real disconnect still gets logged"
companion. trigger-filter 73/73, watcher 21/21, ui 21/21,
wrkflw 3/3, clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug another round of silent-failure holes from review
Another review pass on the diff-aware/watch feature, another batch
of "this thing was lying to the user." Same theme as the previous
five rounds, but the failure modes here are louder and harder to
recover from once they bite, so they go in their own commit.
The TUI's `diff_filter_aborted` flag was a footgun. It got armed
when a toggle aborted an in-flight evaluation, but on the OFF path
the next tick observed `rx is None`, returned early, and never
cleared the flag. The *next* toggle ON spawned a fresh task with a
fresh receiver — and inherited the stale flag. So if that fresh
task genuinely failed, the resulting `Disconnected` was treated as
a self-inflicted abort and silently swallowed. Exactly the failure
mode this whole feature has been built to prevent. Clear the flag
at the start of every new evaluation and pin it with a regression
test that drives the full ON → OFF → tick → ON → fail sequence.
The watcher's `is_relevant_event_kind` had a catch-all `Modify(_)
=> false` that dropped `Modify(Any)` events. It turns out kqueue
(FreeBSD, some macOS configurations) and notify's `PollWatcher`
fallback emit `Modify(Any)` for content changes when the underlying
API can't distinguish data from metadata. So the watcher worked
beautifully on Linux inotify and macOS FSEvents and was *completely
silent* on the other backends. Make the match exhaustive on
`ModifyKind` (no catch-all) so future enum additions surface as a
compile error instead of getting routed to "drop", and treat
`Modify(Any)` as relevant.
The watcher's repo-root canonicalization fallback was the worst of
the three. When `canonicalize` failed, the previous code logged a
warning and continued with the raw path. On macOS that means every
notify event arrives as `/private/var/...`, every `strip_prefix`
against the non-canonical `/var/...` rejects, and the user sees
"Watching..." forever with zero workflows ever firing. A watcher
that *looks* alive but never matches a single event is the absolute
worst possible failure mode for this feature. Make it fatal at
startup. If you can't canonicalize the repo root, you don't get a
watcher.
While at it: stamp `--activity-type` through all three integration
points so workflows with `pull_request: { types: [...] }` filters
actually work in the TUI, the watcher, AND the CLI prefilter
(previously only the CLI bothered, and even then only after the
last round of fixes); make `--diff-base` optional on the run
command so the smart auto-detect (origin/HEAD → main → master →
HEAD~1) is actually reachable instead of being shadowed by a
`default_value = "HEAD"` that silently restricted every `--diff`
invocation to uncommitted-only changes; add an info log when
`get_default_diff_base` falls back to HEAD because the working
tree is dirty, since "I have WIP edits, why is `--diff` reporting
nothing triggered?" was a guaranteed support question; extract
`load_trigger_configs` into the trigger-filter crate so the TUI
and watcher stop maintaining near-duplicate parse-and-partition
loops; tighten the activity-type diagnostic to point at the actual
CLI flag instead of a tautology ("requires one"); warn loudly when
a workflow panics inside the watch loop's `catch_unwind` that the
executor may have left containers, tempdirs, or child processes
uncleaned, because a silent panic-recovery on a long-running watch
session is how you end up with `docker ps -a` looking like a
graveyard; and tune `MAX_SETTLE_ROUNDS` from 10 down to 3 so a
sustained `cargo build` can't delay a debounce drain by 5s while
the user wonders if the watcher hung.
Tests cover all three high-priority bugs explicitly. The
aborted-flag test in particular drives the *exact* sequence the
old code got wrong, because the only thing more embarrassing than
a silent-failure hole in a "no more silent failures" feature is
shipping the same failure mode again three weeks later.
* fix(trigger-filter,watcher): plug three silent-failure holes
Review pass turned up three more places where the trigger filter and
watcher would quietly do the wrong thing instead of telling the user
what was going on. Let's fix that.
First: `parse_events` for the `on: [push, pull_request]` sequence
form was silently dropping non-string entries. A typo like
`on: [push, {pull_request: {branches: [main]}}]` collapsed to
`on: [push]` and the pull_request filter *never applied*. The rest
of the parser has been rejecting this kind of nonsense via
`extract_string_list` for a while — the sequence path just never
got the memo. Now it errors with the offending index and kind, same
as everywhere else.
Second: `explain_filter_failure` was building its branch-mismatch
diagnostic from `filter.branches` only, completely ignoring
`branches_ignore`. A workflow with ONLY `branches-ignore: [main]`
running on `main` rendered as `branch 'main' did not match []` —
factually correct, totally useless. Combine both lists with
`!`-prefixed ignore entries so the diagnostic round-trips to the
surface syntax the user actually wrote. Same treatment for tags,
which had the identical bug.
Third: the watcher's per-cycle `collect_workflow_files` rescan was
swallowing errors with `if let Ok(refreshed) = ...` and silently
reusing the previous snapshot forever. Not wrong — the watch loop
*must* continue on transient failures — but silently falling back
to a stale list with zero diagnostic is exactly the silent-skip
pattern the rest of this PR has been plugging. Log at debug level
so a `--verbose` run leaves a trail.
Regression tests for all three. `cargo test` on the two touched
crates: 101 passed, 0 failed.
* fix(trigger-filter,watcher,wrkflw,ui): plug GHA divergence and CLI foot-guns from review
It turns out the path matcher had a "helpful" fallback that tried the
bare filename when a pattern had no slash in it. Neat trick, except
it's not what GitHub Actions does. `*.rs` would match `src/main.rs`
locally and then silently fail to match the same file on GHA, which
is the single worst outcome for a trigger filter whose entire job is
predicting what the real runner will do. Rip it out. Users who want
"any .rs file anywhere" write `**/*.rs` — that's the GHA idiom and
always has been. Pin the new behavior with a test so nobody resurrects
the fallback during a well-meaning refactor.
While at it, a few CLI foot-guns the same review turned up.
`find_repo_root()` shells out to `git rev-parse --show-toplevel`
synchronously and is *not* covered by `GIT_COMMAND_TIMEOUT` — so a
hung git (credential prompt, stuck NFS mount, corrupt repo) would
cheerfully freeze the tokio reactor on `wrkflw run --diff` and
`wrkflw watch`. The TUI already wrapped this call in `spawn_blocking`;
the CLI just didn't. Fix both call sites.
`wrkflw run --diff .gitlab-ci.yml` ran the GitHub-only trigger
prefilter first and then dumped a confusing "Error parsing workflow"
from deep in the YAML parser. Detect GitLab before the prefilter and
bail out with a message that actually tells the user what went wrong.
`max_concurrency` had a floor of 1 but no ceiling, so
\`--max-concurrency 1000000\` was happily accepted. Each concurrent
workflow carries executor state — containers, tempdirs, child
processes — so unbounded values trivially OOM the host with no
helpful signal. Clamp to 256 and warn loudly on the way down.
The watch loop fire-and-forgets the reporter callback via
\`spawn_blocking\` and then drops the JoinHandle. A panicking callback
vanished into tokio's default panic reporter with zero watch-loop
signal, which is exactly the silent-failure mode this whole PR has
been plugging. Supervisor task now awaits the handle and logs any
panic payload via \`wrkflw_logging::error\`. Not great that the
reporter type still invites \`Fn(WatchEvent)\` directly, but at least
a broken reporter is loud now.
Also: the TUI diff-filter toggle used to pay for two git subprocess
calls even when the workflow list was empty — mirror the watcher's
\`configs.is_empty()\` short-circuit so an empty toggle is free. And
dedup the branches/tags formatting in \`explain_filter_failure\`
behind a \`combined_pattern_sources\` helper so a future diagnostic
tweak can't make the two arms drift apart again.
* fix(trigger-filter,watcher): stop git from stealing TTY and fix symlink ignore hole
Another pass over the PR turned up two ugly ones plus a handful of
smaller silent-failure holes that had been sitting there in plain
sight. Let's deal with all of them at once.
The headline issue: every git subprocess we spawn inherited stdin
from the parent TTY and had no `GIT_TERMINAL_PROMPT=0`. In the TUI,
which runs in raw-terminal mode, that means a backgrounded `git diff`
from the diff-filter toggle will happily consume the user's
keystrokes for the full `GIT_COMMAND_TIMEOUT` window (up to 10
seconds) while ratatui sits there wondering where its input went. On
any repo whose remote wants auth it's even worse — git's askpass
races the UI for every keypress. *This is not great.* Wire
`Stdio::null()` and `GIT_TERMINAL_PROMPT=0` into `git_cmd` so every
subprocess is fully insulated from the parent terminal, and do the
same in `find_repo_root` while we're in there.
The other ugly one: `should_ignore_path` compared events against
`repo_root_canonical` only. That happens to work on macOS because
FSEvents canonicalizes everything, but on Linux with a symlinked
working tree (`/home/alice/proj` pointing at `/mnt/work/proj`),
inotify delivers events rooted at the raw `.watch()` argument, so
`strip_prefix` against the canonical form fails for *every single
event* and the entire ignore filter goes silently dark. You end up
eating every `target/`, `node_modules/` and `.git/` write through the
debouncer. Pass both the raw and canonical forms and try each in
turn — the two regression tests pin both the Linux-symlink and the
macOS `/private/var` paths.
While at it:
- `find_repo_root` had no timeout at all. A hung git there leaks a
blocking-pool thread for the rest of the process's life. Replaced
with a hand-rolled `try_wait` poll + kill-on-deadline so we don't
need tokio in this sync path.
- `get_default_diff_base` trusted `git status` stdout without
checking `status.success()`. On a corrupted repo it would cheerfully
claim a clean tree and fall through. Check the exit code like every
other call site does.
- `parse_event_config` silently accepted non-mapping event values, so
\`on: { push: "main" }\` collapsed to "push with no filters" instead
of surfacing the typo. The one remaining silent-drop hole in a
parser that's otherwise militant about these — fixed, with a
regression test.
- `with_max_concurrency(0)` silently clamped to 1 without the warning
its upper-bound sibling emits. Symmetric now.
- Dropped a stale doc comment on `GlobPattern` that still advertised
the bare-filename fallback `path_matcher` explicitly deleted.
All 79 trigger-filter tests and all 25 watcher tests pass, including
the four new regressions (two symlink cases, the parser typo, and the
ignore-file raw/canonical asymmetry).
* fix(trigger-filter,watcher,wrkflw): plug scalability and platform holes from review
Review flagged three issues that would bite real users the moment this
ships: inotify budget exhaustion on monorepos, a per-cycle git
subprocess storm, and Windows `paths:` filters silently never matching.
Plus a handful of smaller cuts.
**inotify pruning.** `watcher.watch(root, Recursive)` expands into one
inotify watch per directory in the tree, so the recursive-on-root call
was happily registering watches on every directory inside `target/`,
`node_modules/`, and `.git/`. On any serious Rust monorepo that blows
past `fs.inotify.max_user_watches = 8192` before the user edits a
single file, at which point notify fails half-open and the watcher
looks alive but never fires. New `setup_watches()` walks the root,
registers each non-ignored child subtree recursively, and leaves the
high-churn subtrees completely un-watched. The ignore filter on the
event hot path stays too, because macOS FSEvents can still deliver
paths the walker didn't register.
**Branch/tag cache.** Every debounced cycle was shelling out to
`git rev-parse --abbrev-ref HEAD` and `git describe --exact-match
HEAD` — for information that changes on `git checkout` and `git tag`,
i.e. basically never. A 40-file save storm was 80 git spawns for no
reason. New `cached_git_state()` on `WorkflowWatcher` with a 3-second
TTL. Not cache-invalidation-on-`.git/HEAD`-event because that would
require whitelisting `.git/` back past the ignore filter and the TTL
does the same job with a quarter of the complexity.
**Windows path separators.** `canonicalize_changed_paths` handed
backslash-separated strings straight to `path_matcher`, and
`glob::Pattern` with `require_literal_separator: true` expects `/`.
So `paths: ['src/**']` silently didn't match `src\main.rs` and every
Windows user would have seen "0 triggered" with no explanation. New
`normalize_separators()` helper runs after `strip_prefix`.
While at it: `WatcherConfig::with_extra_ignore_dirs()` so users can
add `.terraform`, `coverage`, `.next`, `bazel-bin`, etc. without
waiting for us to land each one as a default. `display_workflow_path()`
renders TRIGGERED/SKIPPED labels as repo-relative instead of the
noisy absolute form. `find_repo_root_detailed()` returns a
classified error (`GitNotInstalled` / `Timeout` / `NotInRepository` /
`Other`) so `wrkflw watch` stops telling users "not inside a git
repository" when the real problem was that git wasn't on PATH or the
subprocess hung — that misdiagnosis was sending people down the
wrong fix path every time. Doc comment on `get_changed_files`
spelling out the rename-as-delete+add behavior since `paths-ignore`
users will eventually trip over it. And a pin test for
`workflow_dispatch`/`schedule`/`repository_dispatch` so a future
refactor that starts defaulting `branches:` can't silently break
manual triggers.
Cancellation token and parallel parse are still deferred — both are
larger surface changes and the review called them out as known
follow-ups. Let's not bundle them here.
* fix(trigger-filter,watcher,ui,wrkflw): fix branches+tags OR, kill silent skips from review
The big one first: a push workflow with BOTH `branches:` and `tags:`
set was being evaluated as AND. So if you had
on:
push:
branches: [main]
tags: ['v*']
the filter would never match *anything*. Branch push? tags axis is
empty (no tag on HEAD), reject. Tag push? branches axis is empty
(detached HEAD, no branch), reject. Elegant symmetry. Completely
wrong.
It turns out GitHub Actions treats that case as OR — the workflow
fires if the branch matches `branches:` OR the tag matches `tags:`.
Which is the only thing that makes sense, because a push is on a
branch *or* a tag, never both simultaneously. The previous
sequential-AND check was literally unsatisfiable for the combo case
and no test covered it, so we shipped a feature that silently
skipped an entire class of real-world workflows.
Extract `ref_filters_pass` that handles all four combinations
explicitly and pin it with four regression tests covering branch
push, tag push, miss, and `tags-ignore` exclusion. Update
`explain_filter_failure` to render "neither ref axis matched" for
the combo case so the diagnostic actually mentions both axes
instead of just one.
While at it, plug the rest of the review punch list.
Retire the `find_repo_root` Option wrapper and migrate every call
site (CLI run prefilter, CLI watch, TUI diff-filter) to
`find_repo_root_detailed`. The wrapper collapsed git-not-installed,
timeout, and not-in-repo into one Option::None, which sent users
down the wrong fix path for three of the four failure modes. The
TUI now surfaces the classified error via `DiffFilterOutcome::
Failure` instead of silently showing "0/N would trigger".
Gate the "diff base = HEAD on dirty tree" info log on a `verbose`
parameter threaded through `get_default_diff_base` and
`auto_detect_context_default_base`. The CLI opts in via its
`--verbose` flag; the TUI passes `false` so a hot-path toggle
doesn't flood the log pane on every invocation. This was a
papercut, not a bug, but it was loud.
Move `process::exit` out of `build_event_context` and its helpers.
The function now returns `Result<EventContext, String>` and the
orchestrator owns the exit policy. Helpers that call `exit` from
deep inside are untestable without spawning subprocesses, and we
were accumulating them for no good reason.
Canonicalize `WatcherConfig::workflow_dir` once in `from_config` so
the trigger cache's `HashMap<PathBuf, _>` keys are stable when the
user passes a relative or symlinked workflow directory. Not a bug
in the current CLI (which always passes an absolute path), but the
foot-gun was there for embedders and it was cheap to remove.
Introduce `ShutdownSignal` — a lightweight cancellation handle
backed by `tokio::sync::watch::channel::<bool>`. `WorkflowWatcher::
run` now takes one as a parameter and observes it via
`tokio::select!` inside the idle wait, so triggering cancellation
wakes the loop immediately instead of parking until the next fs
event. The CLI passes `ShutdownSignal::never()` because it still
relies on `process::exit` from Ctrl+C; long-lived hosts (TUI,
future daemon, tests) construct a real signal.
The first implementation of `ShutdownSignal` used `tokio::sync::
Notify` and had a nasty trigger-before-wait race: `notify_waiters()`
only wakes currently-parked tasks, so a caller that triggered
shutdown and then spawned a task that called `wait()` would park
forever. The watch-channel version stores the latest value, so a
subscriber that joins after trigger sees the fired state on the
very next `wait_for` poll. Pinned by
`wait_after_trigger_sees_triggered_state_immediately` so a future
refactor can't silently reintroduce it.
Split the 1884-line `watcher.rs` into five focused submodules with
their own tests: `ignore` (default ignore set + path filter),
`paths` (canonicalization + separator normalization +
display_workflow_path), `event_kind` (notify event classification),
`trigger_cache` (per-cycle parsing cache), and `setup`
(subtree-by-subtree watch registration). Everything is `pub(crate)`
so the public surface is exactly what `lib.rs` re-exports. Nothing
clever, just stop pretending one file is a reasonable home for
1200 lines of hot-path logic.
Add an end-to-end watcher pipeline test
(`end_to_end_pipeline_delivers_watch_event`) that spins up a
tempdir repo, starts the watcher, touches a file in a pre-existing
subtree, and asserts the callback receives a `WatchEvent` with the
changed file. Plus `run_returns_when_shutdown_signal_is_triggered_
before_any_event` to pin the cancellation interrupt path. Both
tests run under `tokio::task::LocalSet` + `spawn_local` because
`run()` is `!Send` (the executor future holds `dyn ContainerRuntime`).
Document the Ctrl+C resource-leak caveat in `wrkflw watch --help`
— a cycle in flight when the user interrupts will get killed by
the OS without running the executor's cleanup, and users who see
orphaned containers deserve to know why.
All tests green (trigger-filter 84, watcher 39, ui 22, wrkflw 3);
clippy clean.
* fix(trigger-filter,watcher,ui,wrkflw): plug the last silent-skip holes and stop sprinkling constants
Another pass over the diff-aware / watch-mode PR after review. The
recurring theme is the same one the previous 24 fix-ups have been
chasing: warn-and-proceed where we should fail loud, plus a pile
of hardcoded constants that drifted across three call sites because
nobody owned them.
It turns out `--event` without `--diff`/`--changed-files`, and
`pull_request` without `--base-branch`, were both still taking the
log-a-warning-and-run path. In a CI script the warning is invisible
and the user sees a session-long stream of "0 triggered" with no
idea why. Same shape in `get_changed_files`: when `ls-files --others`
failed (safe-directory config, transient permission glitch) we
discarded the error in an `if let Ok(_)` block and dropped every
untracked file from the changed set, silently breaking `paths:`
filters for brand-new files. Not great.
Ctrl+C in `watch` was the other big one. We were using
`ShutdownSignal::never()` and relying on the global cleanup
handler's `process::exit(0)` to tear everything down, which meant
a workflow mid-execution got killed by the OS before the executor's
Docker/tempdir cleanup could run. Install a real
`tokio::signal::ctrl_c` handler that triggers the watcher's
shutdown; the global cleanup still runs after `run()` returns, so
we get a clean two-phase teardown instead of an orphaned-container
parade.
While at it, gather every stray constant (`GIT_COMMAND_TIMEOUT`,
`GIT_STATE_CACHE_TTL`, the glob recompile cost, the hardcoded
`"push"` in the TUI) into a single `TriggerFilterConfig` struct and
thread it through the three call sites (CLI prefilter, watcher hot
loop, TUI diff-filter). Having the same five numbers live in three
different places is not my idea of configurability. The config
also owns a process-wide LRU of compiled trigger configs keyed by
`(path, mtime)`, so toggling the TUI filter or re-running a watch
cycle no longer re-parses every YAML and recompiles every glob.
The watcher picks up three other fixes in the same spirit. The git
state cache now keys on `.git/HEAD`'s mtime on top of the TTL, so a
`git checkout` inside the 3s window is caught on the next cycle
instead of evaluating `branches:` filters against the pre-checkout
branch. The debouncer grows bounded (8192 pending paths by default)
and reports per-cycle drops via `WatchEvent.dropped_events` so a
`cargo clean && cargo build` burst can't grow the pending set to
tens of thousands of PathBufs. And the trigger-cache refresh task
now preserves the *previous* cache on panic instead of returning
`HashMap::new()` — a panicking refresh was previously forcing the
next cycle to re-parse every workflow from scratch, which is the
kind of performance cliff you only notice after the bug report.
Parser learned an allowlist of the documented GHA events and warns
(not errors — GHA adds events faster than we update lists) on any
name outside it, so a typo like `pul_request` surfaces once
instead of producing silent "no matching event" diagnostics
forever. `--changed-files` entries now get validated up front:
absolute paths, Windows drive letters, and `..` components get
rejected with a clear error instead of silently failing every
`paths:` glob.
TUI gains a working event selector. The `diff_filter_event` field
was sitting there hardcoded to `"push"` with a comment about
"future UI" — which is a nice way of saying the TUI was silently
lying about which pull_request/workflow_dispatch/schedule workflows
would run. Shift+D now rotates through the six common events and
re-runs the evaluation against the new one. The log buffer also
grew a cap (5000 lines, enforced at every `mark_logs_for_update`
call site) so a long-lived TUI with a lot of diff-filter toggling
doesn't slowly leak memory.
New tests cover every new behavior: LRU hit + mtime invalidation,
`normalize_user_changed_file` rejecting the bad shapes, the unknown
event warning, the debouncer cap counting drops without counting
deduped paths, HEAD-mtime cache invalidation, and the TUI log cap.
Full workspace still green, clippy silent, fmt clean.
Please don't add more file-local constants to this pipeline. Put
them in `TriggerFilterConfig`.
* fix(trigger-filter,watcher): plug parser, git, and worktree silent-skip holes
Another round of review fallout. Six things were wrong, bundled
because they all share the same failure mode: silently producing
"0 triggered" for the wrong reason and sending users chasing a
flag they never needed.
**`on: {}` and `on: []` were accepted at parse time** and then
surfaced as "Workflow does not listen to any events" from the
evaluator — with no workflow path, no YAML context, nothing. GHA
rejects both at upload time. Reject them here too, at parse,
where the error actually carries the file path and the user has
something to grep for.
**`git diff --name-only` and `git ls-files --others` were
newline-terminated.** So a file called `foo\nbar.txt` got
reported as two bogus half-entries, and any non-ASCII filename
came back as `\302\244`-style octal escapes that no `paths:` glob
could ever match. The fix is three characters: `-z`. Rip out the
old `parse_lines` helper and route everything through a new
`parse_nul_separated` that splits on NUL and tolerates the
trailing NUL git sometimes emits. Pinned with a real-git-repo
end-to-end test that commits a file whose name contains a
literal newline and asserts it survives the round trip.
**`head_mtime` stat'd `.git/HEAD` directly**, which returns None
on every linked worktree because in that layout `.git` is a
regular *file* containing `gitdir: /abs/path/to/real/gitdir`.
The watcher's git-state cache then served stale `(branch, tag)`
pairs until the bare TTL expired. Parse the pointer, resolve
relative paths the same way git does (against the `.git` file's
parent), and stat the real HEAD. Plain repos are unchanged.
**The `buffer_unordered` block in watcher.rs had a comment
claiming "Each future wraps a `tokio::spawn`".** It doesn't.
That's actively *load-bearing* — spawning would detach the
futures from the stream and let max_concurrent_executions be
silently ignored. Rewrite the comment to say what the code
actually does and why, so the next refactor doesn't "fix" it
into an unbounded fork bomb. Please don't do that.
**The path-filter diagnostic told users to "pass --diff or
--changed-files"** even when they had *already* passed `--diff`
and it had just come back empty. Confusion ensues. Add
`EventContext::changed_files_explicit`, set it to `true` in
every constructor that actually ran a diff (the three lib.rs
helpers and the watcher's in-tree path), and split the
diagnostic into two cases so "no diff run yet" and "diff was
empty" read differently.
While at it, mark `EventFilter`, `WorkflowTriggerConfig`, and
`TriggerMatchResult` `#[non_exhaustive]`. No external crate
constructs them today — they only read — so this costs nothing
now and buys room to add fields without a major bump later.
`EventContext` stays exhaustive because the watcher builds it
via struct literal and I'm not rewriting that.
Every fix has a pinned test. `cargo test --workspace` is
629/629. `cargo clippy --all-targets` clean. `cargo fmt` clean.
* fix(trigger-filter,watcher,ui,wrkflw): kill dead plumbing and unlock the pattern cache
Staff-level review of the diff-aware watch branch came back with a
pile of findings. Most were easy, but three of them were actually
embarrassing and needed to go before anyone else looked at this.
First, `PATTERN_CACHE` was holding its mutex *across* the blocking
YAML parse + glob compile in `load_trigger_config`. The whole point
of a process-wide LRU is that concurrent callers hitting different
files don't pay for each other's parses — and we were doing the
exact opposite. One slow network-mounted workflow file would stall
every other caller in the process until it finished. Release the
lock on a miss, parse, re-acquire to insert. Racing duplicate
parses on the same file are harmless because late-writer-wins
produces the same value.
Second, `TriggerFilterConfig` shipped two config knobs —
`git_timeout` and `strict_missing_context` — that had builder
setters, had unit tests pinning the builder setters, and were read
by *absolutely nothing* in the library. The timeout lived as a
file-local const in `git.rs` and strict-mode was implemented
entirely in the CLI. Shipping non-functional config knobs is
precisely the kind of drift this crate has been iteratively
patching, so I deleted them rather than half-wiring them. If
anyone ever needs the git timeout override, thread `Duration`
through `run_git` first and add the field at the same commit.
Third, `KNOWN_GHA_EVENTS` contained `pull_request_comment`, which
is not a real GitHub Actions event. Comments on PRs arrive as
`issue_comment` (PRs are issues). Leaving this in the typo-
detection allowlist silently masked the exact warning the function
exists to produce. One-word fix, regression test pinned.
While at it, I cleaned up the other findings:
- Library logging coupling: `collect_unknown_event_warnings`
(renamed from `warn_on_unknown_events`) and
`get_changed_files_with_warnings` no longer dual-emit via
`wrkflw_logging::warning` AND the return value. They collect
warnings as data; hosts own the rendering policy. Warnings now
ride through a new `WorkflowTriggerConfig::warnings` field in
addition to `EventContext::warnings`. The CLI prefilter renders
both. The one remaining library log call is the verbose-gated
`info!` in `get_default_diff_base` — opt-in, informational, not
dual-emission, and fixing it requires signature cascades I
didn't want in this commit.
- `Debouncer::MAX_SETTLE_ROUNDS` was hardcoded to 3. That was
tuned for the default 500ms debounce (3 × 500 = 1.5s worst
case) but a user passing `--debounce 5000` would see a 15s
worst-case drain delay under sustained churn. Replaced with a
wall-clock `MAX_SETTLE_BUDGET = 1500ms` and a dynamic round
cap derived from the debounce window. Preserves the old
behavior at 500ms and caps long windows correctly.
- `head_mtime` was reachable only via `git::head_mtime`, with the
watcher reaching into a sibling crate's `pub` module. Re-export
at the crate root so the coupling is explicit.
- `normalize_user_changed_file` didn't reject embedded NUL bytes.
NUL is not a valid pathname byte on any supported platform, and
letting one through would only produce downstream confusion.
Reject at the boundary with the rest of the input validation.
- `wrkflw watch` had no CLI flag for `extra_ignore_dirs`. The
watcher supported the knob but users couldn't pass extras
without patching source. Added `--ignore-dir`, comma-separated
or repeated.
- `apply_activity_type` was a one-line function called from one
site. Inlined it.
- `cycle_diff_filter_event` re-ran the evaluation by calling
`toggle_diff_filter` twice in sequence — correct but cryptic.
Extracted `rerun_diff_filter_if_active` so the idiom is named
for what it does.
All 332 tests in the workspace still pass, plus the new
regression pins. Clippy clean, fmt clean.
* fix(ui,watcher): plug TUI warning drop + kill double-toggle fragility
Another review pass. The TUI diff filter was *silently* dropping
every `EventContext::warnings` and every `WorkflowTriggerConfig::warnings`
on the floor — exactly the silent-skip failure mode this entire PR
has been fighting for 28 commits. The library deliberately routes
those diagnostics through struct fields instead of logging them
directly, because hosts own the rendering policy. The CLI prefilter
at crates/wrkflw/src/main.rs does the right thing. The TUI just
didn't.
It turns out that 'hosts own the rendering policy' is a contract
the compiler can't enforce. A git ls-files --others safe-directory
rejection, an 'on: pul_request' typo, a corrupted index — none of
it reached the log pane. User saw '0/N would trigger' with no clue
why.
Let's fix that. DiffFilterReport grows a warnings: Vec<String>
field. evaluate_diff_filter now drains context.warnings via
mem::take and harvests each parsed config's .warnings with the
workflow path prefixed, then check_diff_filter_results renders
them under a 'Diff filter: N warning(s)' line, mirroring the CLI
prefilter byte-for-byte. New regression test pins it.
While at it, rerun_diff_filter_if_active was implemented as two
consecutive toggle_diff_filter() calls. Clever. Also fragile: the
whole thing worked only as long as toggle_diff_filter stayed purely
idempotent in opposing directions, and a future side effect
(metrics, tracing, throttle) would have silently broken the rerun
path with zero compile-time signal. Extract spawn_evaluation() and
abort_in_flight_evaluation() as named helpers so the control flow
matches the intent. The existing abort-flag regression tests still
pin the tricky state-machine invariants through the refactor.
And the Debouncer::MAX_SETTLE_BUDGET doc claimed a hard 1.5s cap
on drain latency that the code doesn't actually enforce when the
user passes a debounce window longer than the budget — a single
tokio::time::sleep is not preemptible and the inner sleep runs the
full user-supplied duration. This is not great, but the alternative
(chopping up the inner sleep) would starve the legitimate coalescing
cases. Soften the doc to describe the real worst case instead of
lying about a cap. The pinning tests already cover both short- and
long-window paths.
Minor: help overlay documents 'd' (toggle diff filter) but not
Shift+D (cycle event). Add the missing line.
* fix(trigger-filter,watcher,wrkflw,ui): plug review holes and enforce warning drain
A code review turned up three real correctness bugs and a handful
of latent traps. Rather than patch them piecemeal, fix the whole
batch in one go so the contract that ties them together — "never
silently drop a diagnostic" — is actually load-bearing in the type
system and not just a convention nobody remembers to follow.
The three correctness bugs first:
1. `cached_git_state` was capturing `head_mtime` *after* the two
git reads. The doc comment claimed this was fine because "a
checkout between the read and the store will show up as an
mtime mismatch on the very next call" — it turns out the
comment was wrong. If a checkout lands between the branch/tag
reads, we store the post-checkout mtime alongside the
pre-checkout branch, and the very next lookup's stat matches
the stored value and happily serves the stale branch until
the TTL expires. Move the mtime capture *before* the git
reads so a race produces a visible mismatch. Regression
test interleaves a real `git checkout` between cache calls.
2. `collect_workflow_files_blocking` was returning
`Err(NoWorkflows)` for an empty directory. Watch mode's
entire value proposition is "react to files that don't exist
yet", so refusing to start on an empty `.github/workflows`
is actively wrong. Worse: mid-session, if the user deleted
every workflow file, the rescan failed, the watch loop fell
back to the stale snapshot, and `refresh_trigger_cache_blocking`
retained the deleted entries in the compiled-pattern cache.
So the evaluator kept running against workflows that no
longer existed. Let's fix that. Empty dir is now `Ok(vec![])`,
the watcher logs an info line and carries on, and the
`retain` step correctly evicts stale entries.
3. The pattern cache was keyed on mtime alone. On FAT32 and
some SMB/NFS configs, two edits within the mtime resolution
window (1-2 seconds) collide, and the second parse gets
served a stale compiled config. Combine `(mtime, len)` as
the key — size changes catch the overwhelming majority of
iterative edits, and it's free because we were already
reading the metadata. A content hash would close the
remaining gap but would require a full file read on every
lookup, which is precisely what the cache exists to avoid.
The bigger structural change is the warning-drain contract.
The trigger-filter crate routes non-fatal diagnostics (unknown
event names, `git ls-files --others` failures) through struct
fields instead of calling `wrkflw_logging::warning` directly,
so hosts own the rendering policy. That's the right design.
But it was enforced entirely by convention, and convention is
not a type system.
Proof: `refresh_trigger_cache_blocking` in the watcher was
silently dropping `WorkflowTriggerConfig::warnings` on the
floor. Nobody noticed because the field was a plain
`Vec<String>` and forgetting to iterate it had no consequences.
Exactly the silent-skip mode the rest of this PR exists to
prevent.
Let's fix that too. New `MustDrainWarnings` newtype with a
`debug_assertions`-gated `Drop` impl that `eprintln!`s when a
non-empty buffer is dropped without being observed via `take()`.
Release builds carry zero overhead. Tests and CI catch the
regression immediately. `EventContext::warnings` and
`WorkflowTriggerConfig::warnings` now use it, every host drains
explicitly (`take()`, not read-only iteration), and the watcher
site that was dropping warnings on the floor now routes them
through `wrkflw_logging::warning` at insert time.
One subtlety: the library's LRU cache stashes a clone of the
parsed config alongside the original it returns to the caller.
The clone's warnings would otherwise trip the Drop check on
eviction. Drain them at insert time — the returned original
still carries the full set for the first observer, and
subsequent cache-hit callers see empty warnings, which is the
correct UX (a first-parse diagnostic should not be re-logged
on every cache hit).
While at it, a few opportunistic cleanups the review flagged:
- `run_trigger_prefilter` returns `Result<PrefilterDecision,
String>` instead of calling `std::process::exit` from half
a dozen sites deep inside the body. The flag matrix is now
unit-testable without spawning a subprocess; three new
tests pin the "directory path", "missing path", and
"non-matching skip" branches.
- The `--strict-filter` / `--no-strict-filter` coalescing
was duplicated across the `run` and `watch` handlers. Extract
`effective_strict_filter(strict, no_strict)` so a third host
can't pick up a slightly different version of the same
bool-toggle pattern.
- `normalize_user_changed_file` was accepting whitespace-only
and empty path components (`src/ /foo.rs`, `src//foo.rs`,
trailing slash). These silently fail every `paths:` glob at
evaluation time — exactly the kind of input the function
exists to reject up front. Reject them with a clear "your
flag was wrong" message.
640+ tests passing across the workspace, clippy clean with
`-D warnings`. Nine new tests cover the fixes, each with a
comment naming the prior failure mode so a future refactor
can't silently regress them.
* fix(trigger-filter,watcher,wrkflw): plug the last six review holes
Review pass flagged eight items. Six code fixes, two coverage
gaps — close all of them so the branch can land without carrying
documented regressions.
The biggest one: the `PATTERN_CACHE` docstring promised that the
CLI, the TUI, and the watcher all shared one parse per (path,
mtime) across the process. It turns out they didn't. Each host
handed `load_trigger_config_cached` a different `PathBuf` shape
— raw user input vs relative `read_dir` output vs canonicalized
notify paths — so each host effectively maintained its own
de-facto private cache entry. The cross-host sharing was pure
aspiration.
Fix is to canonicalize inside the cache lookup via
`canonicalize_allowing_missing` (promoted out of the watcher
crate since three consumers need it now) and rewrite
`workflow_path` back to the caller's original form on return so
UI labels still show what the user typed.
Two more loop-discipline oversights in the watcher.
`debouncer.drain().await` sat outside any `tokio::select!`
against the shutdown signal, so Ctrl+C during an active drain
had to wait up to the full debounce window before the loop
observed it. Every other await in `run()` is cancellation-aware
— this one was the last outlier. And the per-cycle callback
supervisor was a detached `tokio::spawn`: correct on the happy
path, but a stuck reporter accumulates one task per cycle
forever because the runtime's own reaper is the only thing
polling the handle. Now held in a `JoinSet`, reaped via
`try_join_next` at the top of each iteration, with a one-shot
warning when the backlog crosses eight.
The rest is smaller. `--max-pending-events` no longer advertises
a misleading `default: 0` in `--help`; the flag is
`Option<usize>` and `Some(0)` is explicitly rejected since a
zero cap would drop every event. The parser gets a doc note
explaining why the duplicate-event-key concern the reviewer
raised is not reachable on `serde_yaml >= 0.9.4` —
`parse_workflow` errors upstream before ever hitting
`parse_events`. Don't remove it; add the HashSet-walk defence
only if someone swaps in a permissive YAML loader.
Two new tests pin the `PATTERN_CACHE` concurrency contract and
the `--diff-head` without `--diff-base` branch in
`build_event_context`. Both were docstring claims that nothing
actually exercised, which is how these things rot in the first
place.
Version bump to 0.8.0 and the README breaking-change section
are deferred to a follow-up PR so the release ceremony isn't
entangled with the code review.
* fix(trigger-filter,watcher,wrkflw): plug four more review holes
Four things the previous round missed, in order of how much they
would have hurt in production.
The callback supervisor JoinSet in the watcher had a warning
threshold but no hard cap. That's cute when the reporter is
*momentarily* slow, but a truly wedged reporter (deadlocked file
sink, stuck network webhook) just accumulates one supervisor per
cycle forever — the warning never reclaims anything, and memory
grows for the life of the session. Add `SUPERVISOR_HARD_CAP = 128`
and drop the current cycle's `WatchEvent` when we hit it. Loud
error, one-shot latch with hysteresis on the reclaim path, the
watch loop keeps running. Not pretty, but the alternative is
eventually having the OOM killer express an opinion.
`parse_nul_separated` was calling `from_utf8_lossy` on every
record, which silently replaced invalid UTF-8 bytes with U+FFFD
and left the file in the change set under a name that could not
match any real glob. This is the exact "silently lying about
which workflows would run" failure mode the rest of the crate
has been fighting, and it was hiding right inside the NUL
parser. Collect the lossy names in a separate list and surface
them via `EventContext::warnings` so the user at least sees
*which* file was dropped from the filter's view.
Strict-filter default-on (`wrkflw run --event push` now exits
1 without `--diff`/`--changed-files`) is a visible CLI behavior
change and the commit message was the only place it was
documented. That's not great. Add a BREAKING_CHANGES.md entry
with migration examples for each escape hatch, exit-code
semantics, and a pointer at `--no-strict-filter` for scripts
that cannot move yet.
While at it, flesh out the `LOG_BUFFER_CAP = 5000` comment in
the TUI (why not 1000, why not 20000, what the two dominant
log sources are), extract the supervisor constants to module
level so they can be reasoned about, and add tests the review
flagged as missing: a compile-time invariant on the supervisor
cap ordering, a runtime test that shutdown still unblocks
`run()` when the reporter callback is wedged (release-gated
blocking thread so cargo can reap it), and three prefilter
tests pinning the strict-mode rejection paths for
`--event`-alone and `pull_request`-without-`--base-branch`.
2026-04-11 20:27:34 +05:30
### Prefilter exit codes
- `0` — triggers matched, workflow ran to completion (or was skipped
because its triggers did not match the event). A skipped workflow
prints `Workflow skipped: <reason>` and exits 0 to match GitHub
Actions' own "nothing to do" semantics.
- `1` — something went wrong: flag validation, git error, strict-filter
rejection, or workflow execution failure.
### Affected CLI
- `wrkflw run` — new flags: `--event` , `--diff` , `--changed-files` ,
`--diff-base` , `--diff-head` , `--base-branch` , `--activity-type` ,
`--strict-filter` (default on), `--no-strict-filter` .
- `wrkflw watch` — new subcommand. Same strict-filter gate for
`--event pull_request` + missing `--base-branch` .
---
2026-04-22 00:21:09 +05:30
## Shell now matches GitHub Actions invocation (0.8.0)
feat(executor): easy GHA emulation fixes for better compatibility (#82)
* feat(executor): add easy GHA emulation fixes for better compatibility
- Expand github.* context with 13 missing env vars (CI, GITHUB_ACTIONS,
GITHUB_REF_NAME, GITHUB_REF_TYPE, GITHUB_REPOSITORY_OWNER, etc.) and
improve GITHUB_ACTOR to use git config / $USER instead of hardcoded value
- Enforce timeout-minutes at both job level (default 360m per GHA spec)
and step level via tokio::time::timeout
- Implement defaults.run.shell and defaults.run.working-directory with
proper fallback chain: step > job defaults > workflow defaults > bash
- Implement hashFiles() expression function with glob matching, sorted
file hashing (SHA-256), and integration into the substitution pipeline
* fix(executor): harden hashFiles, working-directory, and shell -e
Three issues from code review, all in the "we got the GHA emulation
*almost* right" category:
1. hashFiles() was returning an empty string when no files matched.
GHA returns the SHA-256 of empty input (e3b0c44...), not nothing.
An empty string as a cache key component is the kind of thing
that silently ruins your day. Also, unreadable files were being
skipped without a peep — now we at least warn about it.
2. The working-directory default resolution was doing a naive
Path::join with user-controlled input. If someone writes
`working-directory: ../../../etc` or an absolute path, join
happily replaces the base. Inside a container this is *somewhat*
contained, but in emulation mode it's a real path traversal.
Normalize the path and reject anything that escapes the
workspace.
3. The bash -e flag change (correct per GHA spec) was undocumented.
Scripts that relied on intermediate commands failing without
aborting the step will now break. Document it in
BREAKING_CHANGES.md so users aren't left guessing.
* fix(executor): complete the GHA shell invocation and harden hashFiles
The previous commit added `-e` to bash but stopped there, even
though the BREAKING_CHANGES.md *literally documented* the full GHA
invocation as `bash --noprofile --norc -e -o pipefail {0}`. So we
were advertising behavior we weren't actually implementing. This is
not great.
Without `-o pipefail`, piped commands like `false | echo ok` would
silently succeed, which is exactly the kind of divergence that makes
you distrust an emulator. And without `--noprofile --norc`, user
profile scripts can interfere with reproducibility.
While at it, fix hashFiles error handling — it was silently
swallowing read errors and producing a partial hash, which is worse
than failing because you get a *wrong* cache key with no indication
anything went sideways. preprocess_hash_files and
preprocess_expressions now return Result and the engine surfaces
failures as step errors.
Also add the tests that should have been there from the start:
shell invocation flags, working-directory path traversal rejection,
and defaults cascade (step > job > workflow).
* fix(executor): harden hashFiles, timeout, and shell edge cases
The previous round of GHA emulation fixes left a few holes that
would bite you in production:
hashFiles() would happily glob '../../etc/passwd' and hash whatever
it found outside the workspace. It also loaded entire file contents
into memory before hashing, which is *not great* when someone points
it at a large binary artifact. The glob patterns now reject '..'
traversal, and file contents are streamed into the SHA-256 hasher
via io::copy instead.
timeout-minutes accepted any f64 from YAML, including negative
values, NaN, and infinity — all of which make Duration::from_secs_f64
panic. Non-finite and non-positive values now fall back to the GHA
default of 360 minutes.
Unknown shell values were silently accepted with a '-c' fallback.
Now they emit a warning so you at least *know* something is off.
While at it, replaced the hash_files_read_error_returns_err test
that was testing two Ok paths (despite its name) with proper
path-traversal rejection tests.
* fix(executor): fix shadowed timeout_mins and extract sanitization helper
It turns out the job timeout error path was re-reading the *raw*
timeout_minutes value instead of using the already-sanitized one.
If someone set timeout-minutes to NaN or a negative number, the
sanitization would correctly fall back to 360, but the error
message would happily print "Job exceeded timeout of NaN minutes."
Not great.
Extract sanitize_timeout_minutes() so both the job and step
timeout paths use the same logic instead of duplicating the
is_finite/positive/clamp dance. While at it, add proper tests
for NaN, Infinity, negative, zero, and the max clamp — plus a
test that actually exercises the job-level timeout expiry branch,
which previously had zero coverage.
2026-04-02 18:00:41 +05:30
The `bash` shell now executes with `bash --noprofile --norc -e -o pipefail -c` , matching GitHub Actions behavior. The `sh` shell uses `sh -e -c` . This means:
- Scripts exit immediately on the first command that returns a non-zero exit code (`-e` / errexit)
- In bash, a failure in any command of a pipeline causes the whole pipeline to fail (`-o pipefail` )
- User profile/rc files are not sourced (`--noprofile --norc` )
### Why
GitHub Actions runs `bash` steps with `bash --noprofile --norc -e -o pipefail {0}` . The previous wrkflw behavior of `bash -c` (without `-e` or `pipefail` ) allowed scripts to silently continue past failing commands, which diverged from GHA semantics and could mask real failures.
### Impact
Multi-command `run:` scripts that relied on intermediate commands failing without aborting the step will now fail at the first non-zero exit. Piped commands where an earlier stage fails will also now fail. For example:
```yaml
- run: |
false # This now aborts the step
echo "This no longer runs"
- run: |
false | echo "pipeline now fails too"
```
### Migration
If a step intentionally tolerates command failures, either:
- Append `|| true` to the specific command: `might-fail || true`
- Use `continue-on-error: true` on the step
- Add `set +e` or `set +o pipefail` at the top of the script to opt out selectively
---
2026-04-22 00:21:09 +05:30
## EncryptedSecretStore serialization format (0.8.0)
fix: resolve correctness, security, and parsing bugs across codebase (#73)
* fix: resolve 10 bugs found during full codebase review
- Fix memory leak from Box::leak() in status bar render loop
- Fix AES-GCM nonce reuse vulnerability in encrypted secret storage
- Fix Default impl for EncryptedSecretStore that discarded encryption key
- Fix early return in list command that prevented GitLab pipeline listing
- Fix double validation call in validate_github_workflow
- Wire --show-action-messages CLI flag through ExecutionConfig
- Add serde(rename = "if") to GitLab Rule if_ field for correct deserialization
- Fix potential panic on multibyte paths in workflow tab path shortening
- Include involved job names in circular dependency error messages
- Improve cron syntax validation to check value ranges, steps, and expressions
* fix: address PR review feedback
- Remove dead `_nonce` parameter from `EncryptedSecretStore::from_data`
- Add clarifying comment for inverted show/hide action messages mapping
- Add comprehensive cron validation tests (valid expressions, out-of-range
values, wrong part count, invalid steps, invalid ranges, edge cases)
* fix: resolve 27 bugs found during full codebase verification
A thorough manual verification of every feature uncovered a
*remarkable* collection of bugs hiding in plain sight. The
highlights:
The `strategy.matrix` YAML structure was never parsed. The Job
struct had `matrix` at the top level, but GitHub Actions nests it
under `strategy.matrix`. Serde silently ignored the `strategy`
key, so matrix expansion code existed but could never run. For
absolutely no reason. Introduce a proper `Strategy` struct and
wire it through the executor.
The Step struct was missing `if`, `id`, `working-directory`,
`shell`, and `timeout-minutes` fields. Step-level conditionals
were silently dropped — every step always ran regardless of its
`if` condition. While at it, `continue-on-error` was in the
struct but had no serde rename and was never checked during
execution. Fix all of that.
The validator cheerfully reported cyclic `needs` dependencies as
"Valid". Add DFS cycle detection so `A -> B -> C -> A` is caught
at validation time instead of blowing up at execution time.
Five of eight GitLab CI test fixtures failed to parse because the
model was too rigid: `extends` only accepted arrays (not strings),
`variables` rejected integers, `cache.key` rejected structured
formats, and `script` rejected single strings. Add custom
deserializers following the existing codebase pattern.
The GitHub trigger function leaked the auth token via curl process
arguments visible in `/proc/[pid]/cmdline`. Replace with reqwest,
matching the pattern already used elsewhere. Also add symlink and
path traversal protections in the executor.
Other fixes: hardcoded matrix variable stripping replaced with
proper substitution, `show_action_messages` wired through TUI,
dead `if true {}` removed, default branch detection uses remote
HEAD instead of current branch, cron validator accepts named
days/months, reusable workflow ref validation loosened from OR
to AND, matrix include entries merge into all matching combos.
* fix: harden step-level evaluation, volume checks, and add tests
The PR review turned up a few things that needed fixing before this
was actually ready.
The step-level `if` condition evaluator was silently reusing the
job-level `evaluate_job_condition` function, which knows nothing
about step-scoped expressions like `steps.<id>.outcome`, `success()`,
`failure()`, `always()`, or `cancelled()`. These would fall through
to the generic "unknown condition" path without so much as a warning.
Now they're detected early, a warning is logged, and they default to
true — which is at least *honest* about the limitation.
The volume path traversal check (`..`) was applied to the entire
volume spec string, meaning a perfectly legitimate container path
like `/safe/host:/container/..weird` would get rejected. The check
now only inspects the host path component after splitting on `:`,
which is the part that actually matters for traversal attacks.
While at it, renamed the awkwardly-named `step_name_for_skip` to
just `step_name` in `execute_matrix_job` for consistency with
`execute_job`, and added a BREAKING_CHANGES.md documenting the
EncryptedSecretStore serialization format change.
Added 19 new tests covering matrix include/exclude merge semantics,
step condition evaluation for unsupported expressions, volume path
traversal edge cases, and continue-on-error + step-level if parsing.
* fix: correct condition defaults, path traversal check, and null variable handling
The previous commit defaulted *all* unsupported step-level
condition functions (failure(), cancelled(), always(), success())
to true. It turns out that defaulting failure() and cancelled()
to true is semantically wrong — it means steps guarded by
`if: failure()` will *always* run, even when nothing failed.
That's not a feature, that's a bug.
Default each function to its most likely state: always() and
success() return true, failure() and cancelled() return false.
Not perfect (we still can't track actual step outcomes), but at
least we're not silently running cleanup steps on every build.
The path traversal check was using `contains("..")` which is a
substring match. A directory literally named `..hidden` would
get rejected. Use Path::components() to detect actual ParentDir
components instead of playing string matching games.
While at it, fix deserialize_variables in the GitLab models to
handle YAML null values as empty strings instead of producing
"~\n". Also trim the catch-all serialization output.
* fix: correct cycle detection, condition evaluation, and matrix continue-on-error
The DFS cycle detector in `dfs_detect_cycle` had a genuinely nasty bug:
when a cycle was found, it returned early *without popping itself from
rec_stack*. This left stale entries that corrupted the stack for
subsequent DFS traversals. Net result: cross-edges to already-visited
nodes would be falsely reported as cycles. A→B→A is a cycle, but
D→E→A is just a cross-edge. The old code couldn't tell the difference.
Fix this properly by introducing a separate `in_stack` HashSet for O(1)
membership checks, while keeping the Vec for path reconstruction. Both
are now correctly cleaned up — no early returns skip the cleanup.
While at it, `execute_matrix_job` was silently ignoring `continue-on-error`
on the Err branch. The non-matrix `execute_job` handled it correctly,
but the matrix path would just abort the entire job. Copy-paste bugs
are fun like that. Let's fix that.
The `evaluate_job_condition` status function handling was doing sequential
`contains()` checks with early returns, which meant compound expressions
like `failure() || success()` would match `failure()` first and return
false. Now we scan for all status functions in one pass and pick the
most permissive default when positive functions are present.
Also: `convert_yaml_to_step` was hardcoding `None` for `if_condition`,
`id`, `working_directory`, `shell`, and `timeout_minutes` despite the
YAML potentially having them. And `is_valid_cron_atom` was rejecting
valid POSIX cron syntax like `5/2`.
* refactor(executor): extract step guards into shared helper, fix steps.* default
The step-level if-condition check and continue-on-error handling was
copy-pasted between execute_job and execute_matrix_job with subtly
different control flow — one sets job_success=false and breaks, the
other returns Ok(JobResult{Failure}) immediately. Two copies of the
same logic that *already* disagree is not redundancy, it's a bug
waiting to happen. Let's fix that.
Extract run_step_with_guards() that encapsulates the if-condition
evaluation, execute_step call, and continue-on-error wrapping into
a single StepOutcome enum. Both job execution paths now call this
shared helper.
While at it, fix the condition evaluator defaulting bare steps.*
references to true — "steps.build.outcome == 'failure'" should
*not* optimistically run the step. Now only always() and success()
default to true; everything else (bare step refs, failure(),
cancelled()) conservatively defaults to false.
Also add serde alias "matrix" on Job.strategy so old workflows with
flat matrix: at job level still parse, and document the intentional
or_insert_with in matrix include merging per GitHub Actions spec.
* fix: clean up review findings in step guards, secret store, and test fixture
The PR review flagged three issues worth fixing before merge.
First, run_step_with_guards had a bogus StepStatus::Skipped check
in the abort_job logic. The condition tested for Failure *or*
Skipped, then only actually aborted on Failure — meaning the
Skipped branch did nothing except confuse anyone reading the code.
Simplify to just check Failure directly.
Second, EncryptedSecretStore::from_json would silently fail with a
generic serde error when fed the old serialization format (which
had a shared top-level nonce field). Now it detects the old format
by checking for the "nonce" key and returns a clear error pointing
at BREAKING_CHANGES.md. Added a test for this.
Third, tests/workflows/continue-on-error-test.yml was an orphan
fixture — nothing referenced it. The same content is already
tested inline by parse_continue_on_error_workflow in the parser.
Removed it.
* fix: correct cron day-of-week range, steps. false positive, and Step boilerplate
Three issues from PR review, all straightforward:
The cron validator was rejecting day-of-week value 7, which is a
perfectly valid Sunday alias in both POSIX cron and GitHub Actions.
The max was 6 when it should be 7. The named-value resolver guard
also needed updating from `max == 6` to `max >= 6` so named days
still resolve correctly with the wider range.
The `evaluate_job_condition` heuristic for detecting `steps.*`
references was using a bare `contains("steps.")`, which means an
env var like `env.MY_STEPS_COUNT` would falsely trigger it and
short-circuit to false. Now we check that the character before
"steps." is either start-of-string or non-alphanumeric. Not a
full expression parser, but it stops the obvious false positives.
While at it, add a `Step::with_run` constructor so the GitLab
converter doesn't need three identical 12-field struct literals
that silently break every time someone adds a field to Step.
* fix: harden steps. boundary check, document condition semantics, dedup cycles
The steps. word-boundary heuristic in evaluate_job_condition was
checking for alphanumeric characters before "steps." to avoid false
positives on env vars like "env.MY_STEPS_COUNT". It turns out that
underscore is *not* alphanumeric, so "env._STEPS_CHECK" would
incorrectly trigger the step-reference path and return false.
While at it, the always() && failure() compound expression returning
true got a proper comment explaining *why* that's intentional — we
lack step-status context locally, so we'd rather over-run than
silently skip steps. Not ideal, but honest.
The DFS cycle detector in detect_cyclic_needs could report the same
cycle multiple times depending on HashMap iteration order. Normalize
cycles by rotating the node list to start at the lexicographically
smallest node, then deduplicate via a HashSet. Same cycle from
different entry points now gets reported exactly once.
* fix: squash review nits — double parse, clippy warnings, lost flag
Three leftover issues from the codebase review PR:
The from_json() deserialization was parsing the JSON *twice* — once
into serde_json::Value to sniff for the old nonce field, then again
from the raw string into the actual struct. Parse once, use
from_value() on the already-parsed Value. Not rocket science.
The cycle detector had two clippy warnings: .iter().cloned().collect()
on a slice (just use .to_vec(), please) and .min_by_key() cloning a
double reference instead of comparing properly. Switch to .min_by()
with an explicit cmp.
The show_action_messages flag was being silently dropped in
execute_workflow_cli — hardcoded to false regardless of what the user
asked for. Propagate it through the function signature and the TUI
fallback path so it actually does something.
2026-04-01 23:06:48 +05:30
The `EncryptedSecretStore` struct in `crates/secrets/src/storage.rs` has changed its serialization format:
- The shared `nonce` field has been **removed ** from the struct.
- Each secret now stores its own random nonce **prepended to the ciphertext ** (12 bytes nonce + ciphertext, then base64-encoded).
### Why
The previous design reused a single nonce across all secrets encrypted with the same key. Nonce reuse under AES-GCM is a critical vulnerability — it allows an attacker to XOR ciphertexts to recover plaintext differences and potentially forge authenticated messages.
### Impact
- Any `EncryptedSecretStore` serialized with the old format (containing a top-level `nonce` field) **cannot be deserialized ** by the new code.
- Old ciphertexts (which did not have the nonce prepended) **cannot be decrypted ** by the new code.
### Migration
There is no automatic migration. Users who have persisted encrypted secret stores must re-create them:
1. Decrypt all secrets using the old code (if still accessible).
2. Upgrade to the new version.
3. Re-encrypt and store the secrets.
### Affected API
- `EncryptedSecretStore::from_data` — dropped the `nonce: String` parameter.
- `EncryptedSecretStore` JSON serialization — no longer includes the `nonce` field.
