[pablof7z]

Summary

• render_reminder now deletes the snapshot rows for files it reports after building the reminder. The reminder text is embedded in the user message and carries forward in the conversation history — re-generating it on every subsequent run would duplicate information.
• Snapshots for files that were not externally modified are left intact as valid baselines, so future modifications to those files are still detected.
• Adds ConversationStore::delete_file_snapshots(ids: &[i64]) to support the deletion.

Test plan

• external_modification_produces_diff_reminder — extended to verify a second render_reminder call returns None after the first fires
• unmodified_snapshot_survives_after_modified_snapshot_clears — verifies selective deletion: a stable file's snapshot survives and still triggers when that file is later modified externally

---

Generated by Claude Code

[claude]

Code Review

State: MERGED — leaving notes for future reference and follow-up.

---

Overview

This PR does three things:

1. Moves dynamic per-turn reminders (active tools, shell tasks, file modifications) from the system prompt into the user message, preserving prompt-cache stability.
2. Makes the file-modification reminder fire exactly once by deleting triggered snapshot rows after the reminder is generated.
3. Adds pre-execute hook support alongside the existing pre-tool/post-tool hooks.

All three are well-motivated and correctly implemented. The broken-pipe fix in run_hook is a genuine bug fix that deserved its own mention.

---

Issues

render_reminder has a hidden side effect (file_modifications.rs)

The function is named render_reminder but it now deletes rows from the database after building the string. In Rust, a render_* function is expected to be read-only. A reader who calls this function expecting a pure render will mutate state they didn't anticipate.

The name should reflect the mutation — e.g. take_reminder or render_and_consume_reminder — so callers understand what they're getting. The current name is a correctness trap.

delete_file_snapshots loop is not atomic (store.rs)

for id in ids {
    self.conn.execute("DELETE FROM agent_file_snapshots WHERE id = ?1", params![id])?;
}

If the second or later DELETE fails, the first rows are already gone — the caller's ids slice is now partially consumed. The consequence is benign (the un-deleted snapshots re-trigger on the next run), but in a codebase that treats storage as the contract, a transaction should wrap the loop:

let tx = self.conn.transaction()?;
for id in ids {
    tx.execute("DELETE FROM agent_file_snapshots WHERE id = ?1", params![id])?;
}
tx.commit()

Alternatively, build a dynamic IN (?,?,?) clause for a single atomic statement, which is also more efficient at larger counts.

eprintln! instead of tracing::warn! in fire_pre_execute

eprintln!("[tenex-agent] warn: pre-execute hook '{}' exited non-zero; ...", hook.name);

file_modifications.rs (added in this same PR) uses tracing::warn!. The eprintln! style appears to be pre-existing in project_hooks.rs, but the new fire_pre_execute code should use tracing::warn! consistently. This file owns all hook infrastructure; mixing structured tracing with raw stderr is a maintainability issue.

Non-zero pre-execute exit silently discards stdout

HookRun::Completed { exit_ok, stdout, stderr: _ } => {
    if exit_ok {
        if !stdout.is_empty() {
            injections.push(stdout);
        }
    } else {
        eprintln!("[tenex-agent] warn: ...non-zero...");
        // stdout from the failed run is dropped
    }
}

If a hook exits non-zero but also produced stdout (e.g., a partial result followed by an error), that stdout is silently discarded. The warning message doesn't mention whether output was present. At minimum the warning should note if non-empty stdout was dropped so operators can debug their hooks.

---

Positive Highlights

• Prompt-cache optimization is architecturally sound. Moving variable content out of the system prompt and into the user message means the system prompt bytes are stable across turns, which maximises cache hit rate. The comment explaining this is one of the rare cases where a comment genuinely helps — the WHY is non-obvious.
• Test coverage is excellent. Both external_modification_produces_diff_reminder (fire-once semantics) and unmodified_snapshot_survives_after_modified_snapshot_clears (selective deletion) cover the key invariants cleanly.
• pre_execute_payload_omits_tool_and_args is a clever self-testing pattern — using cat to echo the payload back through the hook runner verifies the serialised wire format without mocking.
• Broken-pipe handling in run_hook is the right fix: the child can legitimately exit before consuming all stdin (e.g., a hook that just prints a line and exits immediately), and treating that as SpawnFailed was wrong.
• AGENTS.md updated — the pre-execute hook semantics are documented alongside the existing hook description.