Skip to content

feat(agent): add pre-execute hook + move turn reminders to user message#124

Merged
pablof7z merged 1 commit into
masterfrom
claude/hooks-pre-execute-branch-relUd
May 31, 2026
Merged

feat(agent): add pre-execute hook + move turn reminders to user message#124
pablof7z merged 1 commit into
masterfrom
claude/hooks-pre-execute-branch-relUd

Conversation

@pablof7z

Copy link
Copy Markdown
Collaborator

Summary

  • Bug fix: active-tools, active-shell-tasks, and file-modifications <system-reminder> blocks were appended to system_prompt via push_str, mutating it per invocation and breaking prompt-cache hits. They now go into the user message instead, keeping system_prompt byte-stable across re-engagements.
  • New feature: pre-execute hook event type — fires once per user-message turn before the LLM call. Cannot block; non-zero exit/timeout/spawn failures are warned and ignored. Stdout is injected into the user message as <system-reminder type="pre-execute-hook">.

Changes

project_hooks.rs

  • Added HookEvent::PreExecute with wire name "pre-execute"
  • Added fire_pre_execute() method and pre_execute_stdin() payload builder (no tool/args fields)
  • Updated config parser to accept "pre-execute" events
  • Fixed latent broken-pipe bug in run_hook (hook ignores stdin and exits early)
  • Updated and added tests

agent_bootstrap/helpers.rs

  • Extended compose_user_message with a reminders: &[String] parameter

agent_bootstrap/mod.rs

  • system_prompt is now immutable (let not let mut)
  • Three reminder blocks collected into turn_reminders: Vec<String> and pre-execute hook injections appended there
  • Single compose_user_message call at the end passes all reminders into the user message

AGENTS.md — documents the three hook event types and their blocking semantics

Test plan

  • cargo test -p tenex-agent passes (153 tests, 0 failures)
  • Pre-execute hooks fire before the LLM call and their stdout appears as <system-reminder type="pre-execute-hook"> in the user turn
  • System prompt is no longer mutated between re-engagements (prompt cache stays warm)

Generated by Claude Code

…to user message

Adds `HookEvent::PreExecute` to the hooks system: fires once per turn
before the LLM call, cannot block, and injects stdout into the user
message as `<system-reminder type="pre-execute-hook">` blocks.

Moves the active-tools, active-shell-tasks, and file-modifications
`<system-reminder>` blocks from the system prompt to the user message
so the system prompt stays byte-stable across re-engagements and
maintains prompt-cache hits.

https://claude.ai/code/session_01PHKUKH8Ejfq3yeE8DuwJob
@pablof7z pablof7z merged commit 022b41a into master May 31, 2026
1 check passed
@claude

claude Bot commented May 31, 2026

Copy link
Copy Markdown

Code Review

Note: This PR is already merged. Feedback is provided for retrospective value and future reference.


Overview

Two well-scoped changes:

  1. Bug fix — runtime reminder blocks (active-tools, active-shell-tasks, file-modifications) were mutating system_prompt per invocation, breaking prompt-cache stability. Moving them to the user message is the correct fix.
  2. Featurepre-execute hook event type that fires once per turn before the LLM call, injecting stdout as a <system-reminder type="pre-execute-hook">.

The core fix is solid and the new feature follows existing patterns cleanly.


Code Quality

Strengths:

  • Making system_prompt a let (not let mut) enforces the invariant at the type level — good.
  • The broken-pipe fix in run_hook is correct. A hook that exits before consuming stdin is not a spawn failure, and the prior behavior would have surfaced a misleading error.
  • pre_execute_stdin() correctly omits tool/args fields — the payload shape is sensible and consistent with the CC hook vocabulary.
  • AGENTS.md is updated with clear blocking semantics for all three event types. ✓

Issues

1. Stderr silently discarded on non-zero pre-execute exit

HookRun::Completed { exit_ok, stdout, stderr: _ } => {
    if !exit_ok {
        eprintln!("[tenex-agent] warn: pre-execute hook '{}' exited non-zero ...", hook.name);
    }
}

The warning tells the operator that the hook failed but not why. If stderr contains the error message, it's useful to include it:

HookRun::Completed { exit_ok, stdout, stderr } => {
    if !exit_ok {
        eprintln!("[tenex-agent] warn: pre-execute hook '{}' exited non-zero{}: pre-execute hooks cannot block",
            hook.name,
            if stderr.is_empty() { String::new() } else { format!(": {stderr}") }
        );
    }
}

This is a minor DX issue — non-zero exit is already semantically ignored, so there's no correctness concern.

2. pre_execute_hooks is a misleading name

let pre_execute_hooks = project_hooks.clone();

This is a full clone of all hooks, not a filtered set. The name implies it only contains PreExecute hooks. hooks_runner or just using project_hooks directly (if it's not moved before this point) would be clearer.

3. Missing negative-path tests for fire_pre_execute

The tests cover the happy path and payload shape well, but there are no tests for:

  • A hook exiting non-zero (should warn and continue, returning no injection)
  • A hook that times out (should warn and continue)

Given the explicit "cannot block" contract, these paths deserve test coverage.


Performance

No concerns. The format! loop in compose_user_message does repeated allocations, but reminders will always be a small slice (≤5 entries), so it's trivially fine.


Security

No new attack surface. Hook commands are operator-controlled via .tenex-hooks.json in the workspace — same trust boundary as the existing pre-tool/post-tool hooks. Hook stdout being injected into user messages is the intended design.


Summary

Area Verdict
Correctness ✅ Sound
Conventions ✅ CLAUDE.md compliant, AGENTS.md updated
Tests ⚠️ Missing negative-path coverage for fire_pre_execute
Naming ⚠️ pre_execute_hooks misleading
Debuggability ⚠️ Stderr discarded on non-zero exit

Clean implementation overall. The three issues above are all minor — none would justify a revert or a follow-up patch on their own, but the stderr/naming points are worth keeping in mind for the next hooks-related change.

Reviewed by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants