feat: Event-driven trigger system for agent workflows#121
Merged
2witstudios merged 6 commits intomainfrom Mar 9, 2026
Merged
Conversation
Implements triggers that fire actions when events occur: agent_idle (inject commands into idle agents), pre_commit and pre_push (gate enforcement via git hooks). Triggers are project-level YAML definitions in .pu/triggers/ with global fallback to ~/.pu/triggers/. Key components: - TriggerDef YAML CRUD following schedule_def.rs pattern - TriggerState tracking on AgentEntry (Active/Gating/Completed/Failed) - Git hook auto-installation on worktree creation - Gate evaluation engine with retry support - agent_idle sequence advancement in scheduler tick - CLI: pu trigger list/show/create/delete, pu gate, --no-trigger flag - Status display with trigger progress indicators Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a trigger system: CLI commands for trigger CRUD and gate evaluation; core trigger definitions with YAML storage; engine support for trigger lifecycle, gate execution and retries; git hook installation for pre-commit/pre-push gate checks; and output/response handling for triggers and gate results. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant Daemon as Engine/Daemon
participant Store as TriggerDefs
participant GateExec as Gate Executor
participant Agent as Agent/Worktree
User->>CLI: spawn (no_trigger: false) / trigger create / gate evaluate
CLI->>Daemon: Request::Spawn or Trigger CRUD or EvaluateGate
activate Daemon
Daemon->>Store: list/find/load trigger defs
Store-->>Daemon: TriggerDef(s)
Daemon->>Agent: spawn agent / install hooks / bind trigger (set trigger_name)
Daemon->>GateExec: evaluate_trigger_gates(worktree_path, TriggerDef)
activate GateExec
GateExec->>Agent: run_gate_command (exec shell in worktree)
Agent-->>GateExec: exit code + stdout/stderr
GateExec-->>Daemon: GateEvalResult (passed/failed + output)
Daemon->>Daemon: update trigger_state / seq_index / inject messages
Daemon-->>CLI: Response::TriggerList / TriggerDetail / GateResult / Spawned
deactivate GateExec
deactivate Daemon
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Fix event name mismatch: normalize hyphenated form (pre-commit) to underscored (pre_commit) in handle_evaluate_gate so git hooks work - Don't overwrite existing git hooks (preserves husky/lefthook/custom) - Escape single quotes in project root path for shell safety in hooks - Bind each agent to a specific trigger by name (trigger_name field on AgentEntry) so sequence is stable across scheduler ticks - Add 60s timeout on gate commands to prevent scheduler blocking - Replace manual TriggerDef↔TriggerInfo conversion with From impls - Remove scan_dir fallback in find_in_dir (surface parse errors) - Replace #[allow(clippy::too_many_arguments)] with CreateTriggerParams Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pu-engine/src/engine.rs (1)
952-1000:⚠️ Potential issue | 🟡 MinorOnly initialize trigger progress when a usable idle trigger is actually bound.
If no idle trigger exists — or the first definition has an empty
sequence— this still writestrigger_seq_index = Some(0),trigger_state = Some(Active), andgate_attempts = Some(0)whiletrigger_namestaysNone. The agent will then show an active trigger forever even thoughevaluate_idle_triggers()skips it.Suggested fix
- let (trigger_name, trigger_total) = if no_trigger { + let (trigger_name, trigger_total) = if no_trigger { (None, None) } else { let pr = project_root.to_string(); let found = tokio::task::spawn_blocking(move || { - let triggers = pu_core::trigger_def::triggers_for_event( + pu_core::trigger_def::triggers_for_event( Path::new(&pr), &pu_core::trigger_def::TriggerEvent::AgentIdle, - ); - triggers.into_iter().next().map(|t| { - let len = t.sequence.len() as u32; - (t.name, len) - }) + ) + .into_iter() + .find(|t| !t.sequence.is_empty()) + .map(|t| (t.name, t.sequence.len() as u32)) }) .await .unwrap_or(None); match found { Some((name, total)) if total > 0 => (Some(name), Some(total)), _ => (None, None), } }; + let trigger_enabled = !no_trigger && trigger_name.is_some(); ... - trigger_seq_index: if no_trigger { None } else { Some(0) }, - trigger_state: if no_trigger { - None - } else { - Some(pu_core::types::TriggerState::Active) - }, + trigger_seq_index: trigger_enabled.then_some(0), + trigger_state: trigger_enabled.then_some(pu_core::types::TriggerState::Active), trigger_total, - gate_attempts: if no_trigger { None } else { Some(0) }, + gate_attempts: trigger_enabled.then_some(0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/engine.rs` around lines 952 - 1000, The manifest initializes trigger progress unconditionally, causing agents to appear active when no usable trigger is bound; modify the AgentEntry construction in this block so trigger_seq_index, trigger_state, and gate_attempts are only Some(...) when a usable trigger is actually bound (i.e., trigger_name.is_some() and trigger_total.unwrap_or(0) > 0); otherwise leave them None (and keep trigger_name None). Locate the AgentEntry initialization near the binding logic (referenced by variables trigger_name, trigger_total, no_trigger and function evaluate_idle_triggers) and gate the fields on that bound condition.
🧹 Nitpick comments (6)
crates/pu-engine/src/git.rs (1)
47-49: Note: This concern applies only if the function is ever called on the main repository.
git rev-parse --git-dirreturns.git(relative) for the main repository but an absolute path for worktrees. For worktree calls (the current and intended use), the returned path is absolute and the file operations work correctly. However, ifinstall_hookswere called on a main repository, the relative path would resolve relative to the process's current working directory rather than the repository location, potentially creating hooks in the wrong location.Given the function is designed for and only called with worktrees (where absolute paths are returned), this is not a current issue, but could be a concern if the function's usage expands in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/git.rs` around lines 47 - 49, install_hooks uses run_git to call "rev-parse --git-dir" and then treats the result as an absolute path, but for a main repo git returns a relative ".git" which would be resolved against the process CWD; ensure the git dir is made absolute by checking if git_dir (from run_git) is a relative path and, if so, joining it with the worktree_path before creating hooks_dir — update the logic around git_dir/hooks_dir in install_hooks (and any helper that calls run_git) so tokio::fs::create_dir_all works on the correct absolute path.crates/pu-engine/src/gate.rs (1)
76-102: Consider handling signal-terminated processes more explicitly.On line 98,
output.status.code().unwrap_or(-1)returns -1 when the process was terminated by a signal (wherecode()returnsNone). This is reasonable, but users may not understand why a gate failed with exit code -1.Consider adding signal information to the output when the process was killed by a signal, though this is a minor enhancement for better debugging.
♻️ Optional: Add signal info for terminated processes
let exit_code = output.status.code().unwrap_or(-1); + #[cfg(unix)] + if exit_code == -1 { + use std::os::unix::process::ExitStatusExt; + if let Some(sig) = output.status.signal() { + // Process was killed by signal; exit_code remains -1 + // The stderr will typically contain relevant info + } + } let stdout = String::from_utf8_lossy(&output.stdout).to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/gate.rs` around lines 76 - 102, run_gate_command currently maps a signal-terminated process to exit code -1 via output.status.code().unwrap_or(-1), which is confusing; update run_gate_command to detect when ExitStatus has no code and append signal information instead: check output.status.code(), and if None on Unix use std::os::unix::process::ExitStatusExt::signal() (or equivalent) to obtain the signal number/name and include that detail in the returned stderr/stdout or in a small human-readable message (e.g., "killed by signal SIGKILL (9)"); keep the existing timeout/GATE_TIMEOUT behavior and only change how signal-terminated cases are represented so callers of run_gate_command can see the signal info instead of just -1.crates/pu-cli/src/output.rs (1)
146-152: Minor: Worktree agents don't display trigger progress.Lines 146-152 show agents within worktrees but don't include the
trigger_progress(a)call, unlike the top-level agents listing (lines 123-128). This could be intentional if worktree agents don't use triggers, but it creates inconsistency.♻️ Optional: Add trigger progress to worktree agent display
for a in wt.agents.values() { println!( - " {:<14} {:<16} {}", + " {:<14} {:<16} {}{}", a.id.dimmed(), a.name, status_colored(a.status, a.exit_code), + trigger_progress(a) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-cli/src/output.rs` around lines 146 - 152, The worktree agent listing loop (iterating wt.agents.values()) prints id, name and status_colored(a.status, a.exit_code) but omits trigger_progress(a), causing inconsistency with the top-level agents display; update the println in that block to include trigger_progress(a) (same call used for top-level agents) as an additional formatted column and adjust the format string/spacing to match the other agents listing so worktree agents show trigger progress too.crates/pu-core/src/trigger_def.rs (2)
135-159: Parse errors are logged to stderr but silently skipped.In
scan_dir(lines 150-152), malformed YAML files produce a warning to stderr but are otherwise ignored. This is reasonable behavior for listing, but users might not notice their trigger files have syntax errors until they try to use them.Consider whether this is the desired UX or if a more prominent warning mechanism would be beneficial.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-core/src/trigger_def.rs` around lines 135 - 159, The current scan_dir function swallows parse failures by printing to stderr with eprintln! and continuing; change it to surface parse errors instead by either (A) replacing eprintln! with the project's structured logger (e.g., log::warn! or tracing::warn!) so warnings go to the app's logging system and include the file path and serde_yml error, or (B) change scan_dir's signature to return a Result<Vec<TriggerDef>, Vec<(PathBuf, serde_yml::Error)>> (or similar) so callers of scan_dir can decide how to report/handle malformed YAML; update uses of scan_dir accordingly and keep the existing behavior for successful TriggerDef parsing. Ensure references to scan_dir, TriggerDef, serde_yml, and the current eprintln! call are replaced/handled.
47-54: Variable substitution doesn't handle edge cases.The
substitute_variablesfunction performs simple string replacement, which works for the common case. However, consider these edge cases:
- Variable values containing
{{could lead to unexpected behavior if substituted values themselves look like placeholders- No escape mechanism if users want literal
{{VAR}}in outputThese are likely acceptable tradeoffs for simplicity, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-core/src/trigger_def.rs` around lines 47 - 54, The substitute_variables function currently does naive repeated replace which can re-introduce placeholders or mis-handle values containing "{{"; change substitute_variables to perform a single-pass, non-recursive substitution by scanning the input once for {{KEY}} tokens (e.g., using a regex or manual scanner) and replacing each match with the corresponding variables.get(KEY).unwrap_or_default() without re-scanning the inserted value, and add support for a simple escape (treat "\{{KEY}}" as a literal "{{KEY}}" in output). Update the doc comment on substitute_variables to explicitly state that substitutions are single-pass (no recursive expansion), values are inserted verbatim (braces in values are not re-parsed), and the "\{{...}}" escape yields a literal placeholder.crates/pu-cli/src/commands/trigger.rs (1)
45-91: Flag order is not preserved when collecting inject and gate actions.The implementation processes all
--injectflags first (lines 51-57), then all--gateflags (lines 58-67). This causes:pu trigger create foo --on pre_commit --inject "/test" --gate "cargo test" --inject "/commit"to produce sequence:
[inject:/test, inject:/commit, gate:cargo test]instead of[inject:/test, gate:cargo test, inject:/commit].This happens because clap parses
--injectand--gateinto separate vectors. To preserve interleaved order, consider collecting actions into a single vector during parsing or refactoring the clap structure. Alternatively, if the current behavior is intentional, document it in the CLI help text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-cli/src/commands/trigger.rs` around lines 45 - 91, The current run_create function builds the trigger sequence by appending all params.injects then all params.gates, losing the original flag order; update run_create (or the CLI parsing for CreateTriggerParams) so actions are collected in flag order—either change the clap parsing to return a single ordered list of actions (inject/gate enum) or, in run_create, reconstruct order from the raw argv/occurrence positions before creating TriggerActionPayload entries for sequence; ensure you update the code paths that push TriggerActionPayload (the inject and GatePayload construction) so they insert actions in the original interleaved order rather than batching injects then gates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pu-cli/src/commands/gate.rs`:
- Around line 16-17: The code sets worktree_path = project_root.clone() but per
the comment it should be the current working directory where the git hook runs;
change the initialization of worktree_path in gate.rs to use the repository's
current working directory (e.g. std::env::current_dir()) instead of cloning
project_root so trigger detection (project_root) and command execution
(worktree_path) are correctly separated; update any downstream uses of
worktree_path to expect a PathBuf from current_dir.
In `@crates/pu-engine/src/engine.rs`:
- Around line 3513-3534: The update_trigger_state function performs synchronous
manifest::update_manifest disk I/O directly on the async scheduler, ignores the
Result, and never calls notify_status_change(); fix by moving the manifest write
into a blocking task (e.g. tokio::task::spawn_blocking or equivalent) and await
its Result, handle/log errors instead of discarding them, and after a successful
manifest update call the engine's notify_status_change() to notify
SubscribeStatus clients; update references to agent.trigger_state,
trigger_seq_index and gate_attempts inside the closure passed to
manifest::update_manifest as before, but ensure the blocking task returns the
write outcome so notify_status_change() is only invoked on success.
- Around line 3505-3510: In inject_text: don't hold the sessions lock across the
await—clone or copy the PTY fd (use handle.master_fd() value) while the sessions
guard is held, then drop the guard before calling pty_host.write_to_fd; also
change inject_text to surface the write result (e.g., return Result<bool, Error>
or bool) so callers can only advance sequences after receiving Ok(true) from the
write, and ensure failures are propagated instead of returning () so
evaluate_idle_triggers won't advance on a disappeared/failed session.
---
Outside diff comments:
In `@crates/pu-engine/src/engine.rs`:
- Around line 952-1000: The manifest initializes trigger progress
unconditionally, causing agents to appear active when no usable trigger is
bound; modify the AgentEntry construction in this block so trigger_seq_index,
trigger_state, and gate_attempts are only Some(...) when a usable trigger is
actually bound (i.e., trigger_name.is_some() and trigger_total.unwrap_or(0) >
0); otherwise leave them None (and keep trigger_name None). Locate the
AgentEntry initialization near the binding logic (referenced by variables
trigger_name, trigger_total, no_trigger and function evaluate_idle_triggers) and
gate the fields on that bound condition.
---
Nitpick comments:
In `@crates/pu-cli/src/commands/trigger.rs`:
- Around line 45-91: The current run_create function builds the trigger sequence
by appending all params.injects then all params.gates, losing the original flag
order; update run_create (or the CLI parsing for CreateTriggerParams) so actions
are collected in flag order—either change the clap parsing to return a single
ordered list of actions (inject/gate enum) or, in run_create, reconstruct order
from the raw argv/occurrence positions before creating TriggerActionPayload
entries for sequence; ensure you update the code paths that push
TriggerActionPayload (the inject and GatePayload construction) so they insert
actions in the original interleaved order rather than batching injects then
gates.
In `@crates/pu-cli/src/output.rs`:
- Around line 146-152: The worktree agent listing loop (iterating
wt.agents.values()) prints id, name and status_colored(a.status, a.exit_code)
but omits trigger_progress(a), causing inconsistency with the top-level agents
display; update the println in that block to include trigger_progress(a) (same
call used for top-level agents) as an additional formatted column and adjust the
format string/spacing to match the other agents listing so worktree agents show
trigger progress too.
In `@crates/pu-core/src/trigger_def.rs`:
- Around line 135-159: The current scan_dir function swallows parse failures by
printing to stderr with eprintln! and continuing; change it to surface parse
errors instead by either (A) replacing eprintln! with the project's structured
logger (e.g., log::warn! or tracing::warn!) so warnings go to the app's logging
system and include the file path and serde_yml error, or (B) change scan_dir's
signature to return a Result<Vec<TriggerDef>, Vec<(PathBuf, serde_yml::Error)>>
(or similar) so callers of scan_dir can decide how to report/handle malformed
YAML; update uses of scan_dir accordingly and keep the existing behavior for
successful TriggerDef parsing. Ensure references to scan_dir, TriggerDef,
serde_yml, and the current eprintln! call are replaced/handled.
- Around line 47-54: The substitute_variables function currently does naive
repeated replace which can re-introduce placeholders or mis-handle values
containing "{{"; change substitute_variables to perform a single-pass,
non-recursive substitution by scanning the input once for {{KEY}} tokens (e.g.,
using a regex or manual scanner) and replacing each match with the corresponding
variables.get(KEY).unwrap_or_default() without re-scanning the inserted value,
and add support for a simple escape (treat "\{{KEY}}" as a literal "{{KEY}}" in
output). Update the doc comment on substitute_variables to explicitly state that
substitutions are single-pass (no recursive expansion), values are inserted
verbatim (braces in values are not re-parsed), and the "\{{...}}" escape yields
a literal placeholder.
In `@crates/pu-engine/src/gate.rs`:
- Around line 76-102: run_gate_command currently maps a signal-terminated
process to exit code -1 via output.status.code().unwrap_or(-1), which is
confusing; update run_gate_command to detect when ExitStatus has no code and
append signal information instead: check output.status.code(), and if None on
Unix use std::os::unix::process::ExitStatusExt::signal() (or equivalent) to
obtain the signal number/name and include that detail in the returned
stderr/stdout or in a small human-readable message (e.g., "killed by signal
SIGKILL (9)"); keep the existing timeout/GATE_TIMEOUT behavior and only change
how signal-terminated cases are represented so callers of run_gate_command can
see the signal info instead of just -1.
In `@crates/pu-engine/src/git.rs`:
- Around line 47-49: install_hooks uses run_git to call "rev-parse --git-dir"
and then treats the result as an absolute path, but for a main repo git returns
a relative ".git" which would be resolved against the process CWD; ensure the
git dir is made absolute by checking if git_dir (from run_git) is a relative
path and, if so, joining it with the worktree_path before creating hooks_dir —
update the logic around git_dir/hooks_dir in install_hooks (and any helper that
calls run_git) so tokio::fs::create_dir_all works on the correct absolute path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95507bf4-2f60-40a2-86ba-0d86f6f7141b
📒 Files selected for processing (16)
crates/pu-cli/src/commands/gate.rscrates/pu-cli/src/commands/mod.rscrates/pu-cli/src/commands/spawn.rscrates/pu-cli/src/commands/trigger.rscrates/pu-cli/src/main.rscrates/pu-cli/src/output.rscrates/pu-core/src/lib.rscrates/pu-core/src/paths.rscrates/pu-core/src/protocol.rscrates/pu-core/src/trigger_def.rscrates/pu-core/src/types.rscrates/pu-engine/src/engine.rscrates/pu-engine/src/gate.rscrates/pu-engine/src/git.rscrates/pu-engine/src/lib.rscrates/pu-engine/src/test_helpers.rs
- Replace handle_save_trigger args with SaveTriggerParams struct - Normalize event names in handle_save_trigger (consistent with gate) - Use shared DEFAULT_GATE_MAX_RETRIES constant (was 3 in idle, 0 in hooks) - Add 5-minute total timeout on evaluate_trigger_gates for git hooks - Wire up Gating state: set before running gate command in idle triggers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pu-engine/src/engine.rs (1)
962-1010:⚠️ Potential issue | 🟠 MajorOnly initialize trigger fields when a trigger was actually bound.
If no
agent_idletrigger exists, this still writestrigger_seq_index = 0,trigger_state = Active, andgate_attempts = 0whiletrigger_namestaysNone. Those agents will look trigger-bound in status output even though nothing can ever advance.Suggested fix
- let (trigger_name, trigger_total) = if no_trigger { + let (trigger_name, trigger_total) = if no_trigger { (None, None) } else { let pr = project_root.to_string(); let found = tokio::task::spawn_blocking(move || { let triggers = pu_core::trigger_def::triggers_for_event( @@ match found { Some((name, total)) if total > 0 => (Some(name), Some(total)), _ => (None, None), } }; + let trigger_bound = trigger_name.is_some(); @@ - trigger_seq_index: if no_trigger { None } else { Some(0) }, - trigger_state: if no_trigger { - None - } else { + trigger_seq_index: if trigger_bound { Some(0) } else { None }, + trigger_state: if trigger_bound { Some(pu_core::types::TriggerState::Active) + } else { + None }, trigger_total, - gate_attempts: if no_trigger { None } else { Some(0) }, + gate_attempts: if trigger_bound { Some(0) } else { None }, no_trigger: if no_trigger { Some(true) } else { None }, trigger_name: trigger_name.clone(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/engine.rs` around lines 962 - 1010, The AgentEntry is currently populated with trigger-related defaults even when no trigger was found; change the initialization so trigger_seq_index, trigger_state, gate_attempts, trigger_total, and no_trigger are only set when a trigger was actually bound (i.e., when trigger_name/trigger_total is Some). Concretely, in the AgentEntry construction (variable agent_entry) replace the existing conditional expressions that use the no_trigger boolean with checks against trigger_name.is_some() or trigger_total.is_some() so that trigger_seq_index = None, trigger_state = None, and gate_attempts = None whenever trigger_name is None (and only populate them when a trigger exists).
♻️ Duplicate comments (2)
crates/pu-engine/src/engine.rs (2)
3529-3550:⚠️ Potential issue | 🟠 MajorMove trigger-state manifest writes off the async path and notify listeners on success.
manifest::update_manifest()is synchronous disk I/O here, its result is discarded, and nonotify_status_change()follows. Slow storage can stall the scheduler, andSubscribeStatusclients will miss trigger progress changes until some unrelated status update happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/engine.rs` around lines 3529 - 3550, update_trigger_state currently performs synchronous disk I/O via manifest::update_manifest on the async path and discards its result, causing scheduler stalls and missed SubscribeStatus updates; change update_trigger_state to run the manifest::update_manifest call inside a blocking context (e.g., tokio::task::spawn_blocking or equivalent) so it does not block the async runtime, check/handle the Result from manifest::update_manifest (log or return error on failure), and on successful write invoke notify_status_change() (or the appropriate status notification function) so listeners receive trigger-state updates; keep the existing mutation logic (agent.trigger_state, trigger_seq_index, gate_attempts) but ensure the disk write is awaited off-thread and only after success you call notify_status_change().
3521-3526:⚠️ Potential issue | 🟠 MajorDon't hold
self.sessionsacross the PTY write, and don't ignore failures.This still awaits
write_to_fd()while the session mutex is held and still returns(). If the session disappears or the write fails, callers at Line 3465 and Line 3505 can advance trigger state as if injection succeeded.Suggested fix
- async fn inject_text(&self, agent_id: &str, text: &str) { - let sessions = self.sessions.lock().await; - if let Some(handle) = sessions.get(agent_id) { - let fd = handle.master_fd(); - let _ = self.pty_host.write_to_fd(&fd, text.as_bytes()).await; - } + async fn inject_text(&self, agent_id: &str, text: &str) -> Result<bool, std::io::Error> { + let fd = { + let sessions = self.sessions.lock().await; + sessions.get(agent_id).map(|handle| handle.master_fd()) + }; + match fd { + Some(fd) => { + self.pty_host.write_to_fd(&fd, text.as_bytes()).await?; + Ok(true) + } + None => Ok(false), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pu-engine/src/engine.rs` around lines 3521 - 3526, The inject_text function currently holds self.sessions.lock().await while awaiting pty_host.write_to_fd and also ignores the write result; change it to only acquire the session handle and master_fd while holding the mutex, then drop the lock before calling self.pty_host.write_to_fd (so do not await while sessions is locked), and propagate or return an error/result instead of swallowing failures so callers (e.g., callers of inject_text) can react if the session vanishes or the write fails; update inject_text's signature to return a Result (or appropriate outcome) and ensure errors from pty_host.write_to_fd are returned rather than ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pu-engine/src/gate.rs`:
- Around line 109-125: The spawned shell process isn't killed when
tokio::time::timeout drops the inner future, so update the Command construction
in the output assignment to enable kill-on-drop: call .kill_on_drop(true) on the
tokio::process::Command before .output() (the block that builds the
Command::new("sh").arg("-c").arg(command).current_dir(cwd).stdout(...).stderr(...)).
This ensures the child process is terminated if the GATE_TIMEOUT fires; leave
the existing timeout/error handling around GATE_TIMEOUT and the command variable
intact.
---
Outside diff comments:
In `@crates/pu-engine/src/engine.rs`:
- Around line 962-1010: The AgentEntry is currently populated with
trigger-related defaults even when no trigger was found; change the
initialization so trigger_seq_index, trigger_state, gate_attempts,
trigger_total, and no_trigger are only set when a trigger was actually bound
(i.e., when trigger_name/trigger_total is Some). Concretely, in the AgentEntry
construction (variable agent_entry) replace the existing conditional expressions
that use the no_trigger boolean with checks against trigger_name.is_some() or
trigger_total.is_some() so that trigger_seq_index = None, trigger_state = None,
and gate_attempts = None whenever trigger_name is None (and only populate them
when a trigger exists).
---
Duplicate comments:
In `@crates/pu-engine/src/engine.rs`:
- Around line 3529-3550: update_trigger_state currently performs synchronous
disk I/O via manifest::update_manifest on the async path and discards its
result, causing scheduler stalls and missed SubscribeStatus updates; change
update_trigger_state to run the manifest::update_manifest call inside a blocking
context (e.g., tokio::task::spawn_blocking or equivalent) so it does not block
the async runtime, check/handle the Result from manifest::update_manifest (log
or return error on failure), and on successful write invoke
notify_status_change() (or the appropriate status notification function) so
listeners receive trigger-state updates; keep the existing mutation logic
(agent.trigger_state, trigger_seq_index, gate_attempts) but ensure the disk
write is awaited off-thread and only after success you call
notify_status_change().
- Around line 3521-3526: The inject_text function currently holds
self.sessions.lock().await while awaiting pty_host.write_to_fd and also ignores
the write result; change it to only acquire the session handle and master_fd
while holding the mutex, then drop the lock before calling
self.pty_host.write_to_fd (so do not await while sessions is locked), and
propagate or return an error/result instead of swallowing failures so callers
(e.g., callers of inject_text) can react if the session vanishes or the write
fails; update inject_text's signature to return a Result (or appropriate
outcome) and ensure errors from pty_host.write_to_fd are returned rather than
ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2685e37e-c31a-405c-a5bb-62e8ff63eef3
📒 Files selected for processing (2)
crates/pu-engine/src/engine.rscrates/pu-engine/src/gate.rs
1. Variable substitution now applied in gate and inject paths 2. Trigger state only set when a trigger is actually bound 3. Git hooks use absolute path to pu binary via current_exe() 4. Warn when multiple agent_idle triggers found (only first used) 5. Simplify no_trigger from Option<bool> to bool 6. Gate commands use kill_on_drop(true) for timeout cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged origin/main which added watch command, pulse endpoint, and suspended-agent display. Resolved conflicts in mod.rs, main.rs, output.rs, paths.rs, and engine.rs by keeping both sides' additions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- gate.rs: Use cwd (not project_root) as worktree_path so gate commands execute in the correct worktree directory - inject_text: Clone PTY fd under lock then drop lock before write; return Result<bool> so callers can fail the trigger on write errors - update_trigger_state: Move manifest write to spawn_blocking, handle errors with tracing::warn, and call notify_status_change on success - kill_on_drop was already addressed in prior commit Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
This was referenced Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agent_idle,pre_commit, andpre_pushevents.pu/triggers/(project) and~/.pu/triggers/(global) with sequence-based actions: inject text into agent PTY or gate commands that must passpu gatepu trigger list/show/create/delete,pu gate,--no-triggerspawn flagKey files
crates/pu-core/src/trigger_def.rs— TriggerDef types, YAML CRUD, variable substitution (23 tests)crates/pu-engine/src/gate.rs— Gate evaluation engine with kill_on_drop timeout cleanup (6 tests)crates/pu-engine/src/engine.rs— Idle trigger advancement in scheduler tick, CRUD handlerscrates/pu-engine/src/git.rs— Git hook installation with absolutepubinary pathcrates/pu-cli/src/commands/{trigger,gate}.rs— CLI commandscrates/pu-cli/src/output.rs— Trigger progress displayReview fixes applied
{{VAR}}) now applied in gate and inject execution pathsno_trigger=false)pubinary viacurrent_exe()for non-PATH environmentsagent_idletriggers found (first is used)no_triggersimplified fromOption<bool>toboolwith serde defaultskill_on_drop(true)to terminate child processes on timeoutinject_textclones PTY fd before dropping sessions lock; returnsResult<bool>so callers only advance on successupdate_trigger_statemoved tospawn_blocking, handles errors, callsnotify_status_changeon successgate.rsworktree_path correctly uses cwd (not project_root) for git hook execution contextTest plan
--no-trigger, verify gate enforcement🤖 Generated with Claude Code