Guard agentflare db files from agent deletion + fix artifact store history tracking - #335
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughArtifactStore gains shared document/blob persistence with filesystem fallback and dashboard routes. Coaching rules gain tiers and host synchronization across CLI and generated rule targets. Shell interception blocks destructive deletion of local agentflare database data. ChangesShared artifact storage
Coaching rule tiers and synchronization
Local data deletion guard
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArtifactDashboard
participant ArtifactStore
participant SharedStore
participant BlobStore
Client->>ArtifactDashboard: request artifact route
ArtifactDashboard->>ArtifactStore: get artifact or version
ArtifactStore->>SharedStore: load document metadata
ArtifactStore->>BlobStore: load referenced snapshot
SharedStore-->>ArtifactStore: return document and history
ArtifactStore-->>ArtifactDashboard: return artifact data
ArtifactDashboard-->>Client: render HTML, JSON, or SSE response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
crates/agentflare-artifacts/src/store.rs (3)
146-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ArtifactMeta.idis persisted empty on the store path.Harmless today because
doc_to_artifact/get_storeread the id fromdoc.path, but any future consumer deserializing the metadata blob gets"". Setting it to the document path costs nothing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/agentflare-artifacts/src/store.rs` around lines 146 - 147, Update the ArtifactMeta construction in the store path to initialize id with the document’s path instead of an empty string. Preserve the existing metadata serialization and ensure the persisted ArtifactMeta.id matches the path used by doc_to_artifact/get_store.
173-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStore path accepts any
update_id, unlike the flat path.
publish_flat(Line 249-253) only reusesupdate_idwhen the artifact directory exists, otherwise it mints a new id. Here an unknown/typo'dupdate_idsilently creates a document at that path. Consider mirroring the flat behavior for consistency:♻️ Suggested tweak
- let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!()); + let id = match req.update_id.as_deref() { + Some(uid) + if store + .doc_get_by_path(DOC_PROJECT, uid) + .map_err(Self::store_conn_err)? + .is_some() => + { + uid.to_string() + } + _ => nanoid::nanoid!(), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/agentflare-artifacts/src/store.rs` around lines 173 - 176, Update publish_store to reuse req.update_id only when the corresponding artifact directory already exists, matching publish_flat; otherwise generate a fresh nanoid. Preserve the current path and response handling for valid existing IDs.
649-656: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a no-op republish test to lock in dedup behavior.
The new suite covers publish/get/versions/list/delete/diff, but not the dedup branch (Line 194) — the one place where an algorithm mismatch would silently inflate versions and history. A test that republishes identical content and asserts the version stays put would catch it.
Also applies to: 761-855
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/agentflare-artifacts/src/store.rs` around lines 649 - 656, Add a test near the existing publish/version tests that uses the doc_store helper to publish identical content twice, then asserts the second publish is a no-op and the artifact version remains unchanged. Ensure the test specifically exercises the deduplication branch in the publish flow without altering the existing test coverage.src/dashboard/artifacts.rs (1)
20-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
open_store()runs at router-construction time and is duplicated.Two notes: (1) the store handle is opened eagerly whenever
router()is built (including insrc/dashboard/server.rs's test that constructs the router), which touches the real~/.agentflarestore; (2) the fallback block duplicatessrc/artifacts.rs— see the consolidated comment.Also applies to: 109-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/artifacts.rs` around lines 20 - 32, Update open_store and its router-construction call path so artifact storage is not opened eagerly when the dashboard router is built, including server tests. Reuse the existing store-opening and fallback logic from src/artifacts.rs instead of duplicating the ArtifactStore::with_store and ArtifactStore::new branches, while preserving the ArtifactState base_url and shared store initialization behavior.src/artifacts.rs (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared-store-with-flat-file-fallback selection into one helper. Three call sites repeat the same
crate::store::open()→ArtifactStore::with_store→ stderr log →~/.agentflare/artifactsfallback, each with its own log prefix; a divergence in any one of them changes where artifacts land.
src/artifacts.rs#L5-L16: replace theelsebranch with a call to a new shared helper (e.g.crate::artifacts::default_store()), keeping thedir-provided branch as-is.src/cli/handoff.rs#L92-L103: replace theNonearm with the shared helper.src/dashboard/artifacts.rs#L20-L32: buildstorefrom the shared helper insideopen_store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/artifacts.rs` around lines 5 - 16, Extract the repeated shared-store-with-flat-file-fallback logic into a single helper in the artifacts module, such as default_store(), including the existing fallback path and stderr logging. In src/artifacts.rs lines 5-16, replace only the dir-less branch with this helper and preserve the provided-dir branch; in src/cli/handoff.rs lines 92-103, replace the None arm; and in src/dashboard/artifacts.rs lines 20-32, use the helper when constructing store inside open_store.src/hook_redirect.rs (1)
61-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the statement-splitting and targeting checks into small helpers.
Static analysis flags this function as high complexity. The nested
flat_mapchain plus the sequential boolean flags (is_destructive_verb,targets_agentflare_dir,targets_db_or_whole_dir) could be split into asplit_statements(command)helper and a couple of named predicate functions for readability, without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hook_redirect.rs` around lines 61 - 105, Reduce complexity in destructive_data_file_reason by extracting statement splitting into a split_statements helper and moving the destructive-verb, .agentflare-target, and database/whole-directory checks into named predicate helpers. Preserve the current statement delimiters, normalization, matching behavior, and blocked-message result.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/agentflare-store/src/documents.rs`:
- Around line 193-194: Update the unchanged calculation in the document upsert
flow to compare blob hashes only when opts.blob_hash is provided; treat an
omitted hash as unchanged for existing blob-backed documents while preserving
content comparison and explicit hash-change detection. Keep the track_history
condition unchanged.
In `@src/dashboard/artifacts.rs`:
- Around line 53-56: Update the artifact handlers around the synchronous store
calls and render operations, including the paths at the shown locations, to
execute SQLite/filesystem work via spawn_blocking, matching the existing index
handler’s pattern. Move each state.store access and associated synchronous
rendering into the blocking closure, await its result, and preserve the current
not-found response and handler responses.
- Around line 92-100: Update the SSE bridge task around the subscribe loop to
use timed receives (such as recv_timeout) instead of blocking indefinitely on
rx.recv(), periodically checking whether the unbounded channel’s tx has been
closed and exiting when it is. Also configure the Sse response with
keep_alive(...) so idle live connections remain active through intermediaries.
- Around line 49-83: Apply the existing valid_id guard at the start of
artifact_page, artifact_version_page, and versions_json, returning the same
invalid-ID response used by artifact_live before calling store.get,
store.get_version, or store.versions. Keep valid IDs on their current rendering
and JSON paths.
In `@src/hook_redirect.rs`:
- Around line 78-105: Update destructive_data_file_reason to split statements on
the lone '&' operator, removing the redundant && splitting while preserving
empty-statement handling. Normalize the first command token so path-qualified
binaries and the backslash-normalized /rm form are recognized by comparing its
basename to the destructive verbs, while retaining the existing agentflare path
and database/recursive deletion checks.
In `@src/mcp_server.rs`:
- Around line 419-426: Update the artifact store initialization in the shown
match block to use the instance’s self.store_override when opening the store
instead of always calling crate::store::open(). Preserve the existing fallback
to the flat-file ArtifactStore when opening the selected override fails,
ensuring private instances remain isolated.
---
Nitpick comments:
In `@crates/agentflare-artifacts/src/store.rs`:
- Around line 146-147: Update the ArtifactMeta construction in the store path to
initialize id with the document’s path instead of an empty string. Preserve the
existing metadata serialization and ensure the persisted ArtifactMeta.id matches
the path used by doc_to_artifact/get_store.
- Around line 173-176: Update publish_store to reuse req.update_id only when the
corresponding artifact directory already exists, matching publish_flat;
otherwise generate a fresh nanoid. Preserve the current path and response
handling for valid existing IDs.
- Around line 649-656: Add a test near the existing publish/version tests that
uses the doc_store helper to publish identical content twice, then asserts the
second publish is a no-op and the artifact version remains unchanged. Ensure the
test specifically exercises the deduplication branch in the publish flow without
altering the existing test coverage.
In `@src/artifacts.rs`:
- Around line 5-16: Extract the repeated shared-store-with-flat-file-fallback
logic into a single helper in the artifacts module, such as default_store(),
including the existing fallback path and stderr logging. In src/artifacts.rs
lines 5-16, replace only the dir-less branch with this helper and preserve the
provided-dir branch; in src/cli/handoff.rs lines 92-103, replace the None arm;
and in src/dashboard/artifacts.rs lines 20-32, use the helper when constructing
store inside open_store.
In `@src/dashboard/artifacts.rs`:
- Around line 20-32: Update open_store and its router-construction call path so
artifact storage is not opened eagerly when the dashboard router is built,
including server tests. Reuse the existing store-opening and fallback logic from
src/artifacts.rs instead of duplicating the ArtifactStore::with_store and
ArtifactStore::new branches, while preserving the ArtifactState base_url and
shared store initialization behavior.
In `@src/hook_redirect.rs`:
- Around line 61-105: Reduce complexity in destructive_data_file_reason by
extracting statement splitting into a split_statements helper and moving the
destructive-verb, .agentflare-target, and database/whole-directory checks into
named predicate helpers. Preserve the current statement delimiters,
normalization, matching behavior, and blocked-message result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8d7d350-58ab-4439-b757-0d15a9aff0e0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/agentflare-artifacts/Cargo.tomlcrates/agentflare-artifacts/src/store.rscrates/agentflare-store/src/documents.rssrc/artifacts.rssrc/cli/handoff.rssrc/dashboard/artifacts.rssrc/dashboard/mod.rssrc/dashboard/server.rssrc/hook_redirect.rssrc/mcp_server.rs
Comment on lines
+193
to
+194
| let unchanged = old_content == content && old_blob_hash == opts.blob_hash; | ||
| if opts.track_history && !unchanged { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Any caller that re-upserts an existing doc without blob_hash?
rg -nP -C6 'doc_upsert_with_opts\(' src crates --glob '!**/documents.rs'Repository: getappz/agentflare
Length of output: 5961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== crates/agentflare-store/src/documents.rs (around lines 150-250) ==\n'
sed -n '150,250p' crates/agentflare-store/src/documents.rs
printf '\n== Caller sites with doc_upsert_with_opts and blob_hash usage ==\n'
rg -n -C4 'doc_upsert_with_opts\(|blob_hash:' src crates --glob '!**/documents.rs'Repository: getappz/agentflare
Length of output: 11557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== crates/agentflare-store/src/documents.rs (full upsert logic slice) ==\n'
sed -n '180,240p' crates/agentflare-store/src/documents.rs
printf '\n== src/asset_store.rs ==\n'
sed -n '68,110p' src/asset_store.rs
printf '\n== src/mcp_server/asset.rs ==\n'
sed -n '92,125p' src/mcp_server/asset.rs
printf '\n== src/mcp_server/handoff.rs ==\n'
sed -n '126,155p' src/mcp_server/handoff.rs
printf '\n== crates/agentflare-artifacts/src/store.rs ==\n'
sed -n '208,235p' crates/agentflare-artifacts/src/store.rsRepository: getappz/agentflare
Length of output: 9011
Treat omitted blob_hash as unchanged on blob-backed docs
Re-upserting an existing blob-backed doc without blob_hash makes Some(old) != None, so history is recorded on every call even though the blob column is left untouched. Compare the blob hash only when the caller supplies one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/agentflare-store/src/documents.rs` around lines 193 - 194, Update the
unchanged calculation in the document upsert flow to compare blob hashes only
when opts.blob_hash is provided; treat an omitted hash as unchanged for existing
blob-backed documents while preserving content comparison and explicit
hash-change detection. Keep the track_history condition unchanged.
Comment on lines
+49
to
+83
| async fn artifact_page( | ||
| State(state): State<ArtifactState>, | ||
| Path(id): Path<String>, | ||
| ) -> Response { | ||
| let Ok(artifact) = state.store.get(&id) else { | ||
| return (StatusCode::NOT_FOUND, "artifact not found").into_response(); | ||
| }; | ||
| let html = agentflare_artifacts::render_artifact_page(&artifact, true, &state.base_url); | ||
| ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() | ||
| } | ||
|
|
||
| async fn artifact_version_page( | ||
| State(state): State<ArtifactState>, | ||
| Path(VersionPath { id, version }): Path<VersionPath>, | ||
| ) -> Response { | ||
| let Ok(artifact) = state.store.get_version(&id, version) else { | ||
| return (StatusCode::NOT_FOUND, "artifact version not found").into_response(); | ||
| }; | ||
| let html = agentflare_artifacts::render_artifact_page(&artifact, false, &state.base_url); | ||
| ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() | ||
| } | ||
|
|
||
| async fn versions_json( | ||
| State(state): State<ArtifactState>, | ||
| Path(id): Path<String>, | ||
| ) -> Response { | ||
| match state.store.versions(&id) { | ||
| Ok(history) => ( | ||
| [(header::CONTENT_TYPE, "application/json")], | ||
| serde_json::to_string_pretty(&history).unwrap_or_else(|_| "[]".into()), | ||
| ) | ||
| .into_response(), | ||
| Err(_) => (StatusCode::NOT_FOUND, "artifact not found").into_response(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing valid_id check on the page/version/versions routes.
Only artifact_live validates the id (Line 89). When open_store() falls back to the flat-file ArtifactStore (Line 25), get/get_version/versions resolve base_path.join(id), and axum percent-decodes path segments, so %2e%2e%2f… can escape the artifacts directory. Apply the same guard used by the artifact server:
🛡️ Proposed fix
async fn artifact_page(
State(state): State<ArtifactState>,
Path(id): Path<String>,
) -> Response {
+ if !agentflare_artifacts::valid_id(&id) {
+ return (StatusCode::NOT_FOUND, "invalid id").into_response();
+ }
let Ok(artifact) = state.store.get(&id) else {…and the equivalent guard at the top of artifact_version_page and versions_json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/artifacts.rs` around lines 49 - 83, Apply the existing valid_id
guard at the start of artifact_page, artifact_version_page, and versions_json,
returning the same invalid-ID response used by artifact_live before calling
store.get, store.get_version, or store.versions. Keep valid IDs on their current
rendering and JSON paths.
Comment on lines
+53
to
+56
| let Ok(artifact) = state.store.get(&id) else { | ||
| return (StatusCode::NOT_FOUND, "artifact not found").into_response(); | ||
| }; | ||
| let html = agentflare_artifacts::render_artifact_page(&artifact, true, &state.base_url); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Blocking SQLite/filesystem reads on the async runtime thread.
index correctly wraps render_index in spawn_blocking, but these three handlers call the synchronous store directly from async context. Under concurrent requests this stalls the executor. Wrap them the same way index does.
Also applies to: 64-67, 75-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/artifacts.rs` around lines 53 - 56, Update the artifact
handlers around the synchronous store calls and render operations, including the
paths at the shown locations, to execute SQLite/filesystem work via
spawn_blocking, matching the existing index handler’s pattern. Move each
state.store access and associated synchronous rendering into the blocking
closure, await its result, and preserve the current not-found response and
handler responses.
Comment on lines
+92
to
+100
| let rx = state.store.subscribe(&id); | ||
| let (tx, async_rx) = tokio::sync::mpsc::unbounded_channel::<String>(); | ||
| tokio::task::spawn_blocking(move || { | ||
| while let Ok(event) = rx.recv() { | ||
| if tx.send(event).is_err() { | ||
| break; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The SSE bridge task parks a blocking thread until an event arrives, even after the client disconnects.
ArtifactStore::broadcast only wakes subscribers when a refresh actually happens, so rx.recv() blocks indefinitely for artifacts that are never republished; the tx.send error check can't run until then. Each abandoned /live connection therefore holds a thread from the (bounded) blocking pool permanently. Use recv_timeout in the loop so the task can notice a closed tx and exit:
🛡️ Proposed fix
tokio::task::spawn_blocking(move || {
- while let Ok(event) = rx.recv() {
- if tx.send(event).is_err() {
- break;
- }
- }
+ loop {
+ match rx.recv_timeout(std::time::Duration::from_secs(15)) {
+ Ok(event) => {
+ if tx.send(event).is_err() {
+ break;
+ }
+ }
+ // Nothing published yet — bail out once the client is gone.
+ Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {
+ if tx.is_closed() {
+ break;
+ }
+ }
+ Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => break,
+ }
+ }
});Also worth adding .keep_alive(...) to the Sse response so idle connections aren't dropped by intermediaries.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let rx = state.store.subscribe(&id); | |
| let (tx, async_rx) = tokio::sync::mpsc::unbounded_channel::<String>(); | |
| tokio::task::spawn_blocking(move || { | |
| while let Ok(event) = rx.recv() { | |
| if tx.send(event).is_err() { | |
| break; | |
| } | |
| } | |
| }); | |
| let rx = state.store.subscribe(&id); | |
| let (tx, async_rx) = tokio::sync::mpsc::unbounded_channel::<String>(); | |
| tokio::task::spawn_blocking(move || { | |
| loop { | |
| match rx.recv_timeout(std::time::Duration::from_secs(15)) { | |
| Ok(event) => { | |
| if tx.send(event).is_err() { | |
| break; | |
| } | |
| } | |
| // Nothing published yet — bail out once the client is gone. | |
| Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { | |
| if tx.is_closed() { | |
| break; | |
| } | |
| } | |
| Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => break, | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/artifacts.rs` around lines 92 - 100, Update the SSE bridge task
around the subscribe loop to use timed receives (such as recv_timeout) instead
of blocking indefinitely on rx.recv(), periodically checking whether the
unbounded channel’s tx has been closed and exiting when it is. Also configure
the Sse response with keep_alive(...) so idle live connections remain active
through intermediaries.
Comment on lines
+78
to
+105
| fn destructive_data_file_reason(command: &str) -> Option<String> { | ||
| for statement in command.split([';', '\n']).flat_map(|s| s.split("&&")).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) { | ||
| let trimmed = statement.trim().to_lowercase().replace('\\', "/"); | ||
| let Some(first_word) = trimmed.split_whitespace().next() else { | ||
| continue; | ||
| }; | ||
| let is_destructive_verb = matches!(first_word, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir"); | ||
| if !is_destructive_verb { | ||
| continue; | ||
| } | ||
| let targets_agentflare_dir = | ||
| trimmed.contains(".agentflare/") || trimmed.ends_with(".agentflare"); | ||
| if !targets_agentflare_dir { | ||
| continue; | ||
| } | ||
| // Either a specific *.db*/-wal/-shm file, or a recursive/whole-dir | ||
| // delete of .agentflare itself (which would take the db files with it). | ||
| let targets_db_or_whole_dir = trimmed.contains(".db") | ||
| || trimmed.ends_with(".agentflare") | ||
| || trimmed.contains(" -r ") | ||
| || trimmed.contains(" -rf") | ||
| || trimmed.contains("-recurse"); | ||
| if targets_db_or_whole_dir { | ||
| return Some("deleting agentflare's local data files (~/.agentflare/*.db, *.db-wal, *.db-shm, or the .agentflare directory itself) is blocked — they hold tracked items, artifacts, and secrets with no automatic backup. If a file genuinely needs to be removed, ask the user to run the command themselves.".to_string()); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detection can be bypassed by sudo/path-qualified binaries and a lone &.
Two concrete gaps in the statement/verb matching let a genuinely destructive delete slip through undetected:
matches!(first_word, "rm" | "del" | ...)requires an exact match, sosudo rm ~/.agentflare/store.db,/bin/rm ~/.agentflare/store.db, or./rm ...never match (first word is"sudo"/"/bin/rm"/"./rm"). Ironically, the\→/normalization on line 80 (added for Windows paths) also turns bash's alias-bypass idiom\rminto/rm, which then fails the exact match too.- The statement splitter (line 79) only breaks on
;,\n,&&,||,|— it never splits on a lone&. A single&is a real bash/POSIX control operator that separates commands (it backgrounds the first one), sosleep 1 & rm ~/.agentflare/store.dbis treated as one statement with first word"sleep", and the trailingrmis never inspected.
Both are easy, realistic ways an agent (or a copy-pasted one-liner) could trigger the exact incident this guard exists to prevent.
🔒 Proposed fix for both gaps
- for statement in command.split([';', '\n']).flat_map(|s| s.split("&&")).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) {
+ for statement in command.split([';', '\n', '&']).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) {
let trimmed = statement.trim().to_lowercase().replace('\\', "/");
let Some(first_word) = trimmed.split_whitespace().next() else {
continue;
};
- let is_destructive_verb = matches!(first_word, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir");
+ let verb = first_word.rsplit('/').next().unwrap_or(first_word);
+ let is_destructive_verb = matches!(verb, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir");Note splitting the initial char set on & also subsumes && (each && becomes two adjacent & splits, yielding an empty statement that's already skipped), so the separate .split("&&") step can be dropped. sudo/env/command-prefixed invocations are a further residual gap worth a follow-up if worth the complexity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn destructive_data_file_reason(command: &str) -> Option<String> { | |
| for statement in command.split([';', '\n']).flat_map(|s| s.split("&&")).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) { | |
| let trimmed = statement.trim().to_lowercase().replace('\\', "/"); | |
| let Some(first_word) = trimmed.split_whitespace().next() else { | |
| continue; | |
| }; | |
| let is_destructive_verb = matches!(first_word, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir"); | |
| if !is_destructive_verb { | |
| continue; | |
| } | |
| let targets_agentflare_dir = | |
| trimmed.contains(".agentflare/") || trimmed.ends_with(".agentflare"); | |
| if !targets_agentflare_dir { | |
| continue; | |
| } | |
| // Either a specific *.db*/-wal/-shm file, or a recursive/whole-dir | |
| // delete of .agentflare itself (which would take the db files with it). | |
| let targets_db_or_whole_dir = trimmed.contains(".db") | |
| || trimmed.ends_with(".agentflare") | |
| || trimmed.contains(" -r ") | |
| || trimmed.contains(" -rf") | |
| || trimmed.contains("-recurse"); | |
| if targets_db_or_whole_dir { | |
| return Some("deleting agentflare's local data files (~/.agentflare/*.db, *.db-wal, *.db-shm, or the .agentflare directory itself) is blocked — they hold tracked items, artifacts, and secrets with no automatic backup. If a file genuinely needs to be removed, ask the user to run the command themselves.".to_string()); | |
| } | |
| } | |
| None | |
| } | |
| fn destructive_data_file_reason(command: &str) -> Option<String> { | |
| for statement in command.split([';', '\n', '&']).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) { | |
| let trimmed = statement.trim().to_lowercase().replace('\\', "/"); | |
| let Some(first_word) = trimmed.split_whitespace().next() else { | |
| continue; | |
| }; | |
| let verb = first_word.rsplit('/').next().unwrap_or(first_word); | |
| let is_destructive_verb = matches!(verb, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir"); | |
| if !is_destructive_verb { | |
| continue; | |
| } | |
| let targets_agentflare_dir = | |
| trimmed.contains(".agentflare/") || trimmed.ends_with(".agentflare"); | |
| if !targets_agentflare_dir { | |
| continue; | |
| } | |
| // Either a specific *.db*/-wal/-shm file, or a recursive/whole-dir | |
| // delete of .agentflare itself (which would take the db files with it). | |
| let targets_db_or_whole_dir = trimmed.contains(".db") | |
| || trimmed.ends_with(".agentflare") | |
| || trimmed.contains(" -r ") | |
| || trimmed.contains(" -rf") | |
| || trimmed.contains("-recurse"); | |
| if targets_db_or_whole_dir { | |
| return Some("deleting agentflare's local data files (~/.agentflare/*.db, *.db-wal, *.db-shm, or the .agentflare directory itself) is blocked — they hold tracked items, artifacts, and secrets with no automatic backup. If a file genuinely needs to be removed, ask the user to run the command themselves.".to_string()); | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hook_redirect.rs` around lines 78 - 105, Update
destructive_data_file_reason to split statements on the lone '&' operator,
removing the redundant && splitting while preserving empty-statement handling.
Normalize the first command token so path-qualified binaries and the
backslash-normalized /rm form are recognized by comparing its basename to the
destructive verbs, while retaining the existing agentflare path and
database/recursive deletion checks.
Comment on lines
+419
to
+426
| let store = match crate::store::open() { | ||
| Ok(s) => std::sync::Arc::new(agentflare_artifacts::ArtifactStore::with_store(s)), | ||
| Err(e) => { | ||
| eprintln!("[artifacts] fallback to flat-file store: {e}"); | ||
| let dir = crate::paths::home().join(".agentflare").join("artifacts"); | ||
| std::sync::Arc::new(agentflare_artifacts::ArtifactStore::new(dir)) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'fn open\(|fn store_path' src/store.rs
rg -nP -C3 'store_override' srcRepository: getappz/agentflare
Length of output: 3857
🏁 Script executed:
sed -n '390,450p' src/mcp_server.rs
sed -n '670,700p' src/mcp_server.rs
sed -n '80,115p' src/mcp_server.rs
sed -n '1,60p' src/store.rsRepository: getappz/agentflare
Length of output: 8317
🏁 Script executed:
rg -n -C3 'for_test|artifacts_dir_override|ensure_artifact_server|ArtifactStore::with_store|ArtifactStore::new' src/mcp_server.rs src/mcp_serverRepository: getappz/agentflare
Length of output: 13460
🏁 Script executed:
sed -n '520,590p' src/mcp_server.rs
sed -n '1,120p' src/mcp_server/flare_docs.rs
sed -n '120,220p' src/mcp_server.rsRepository: getappz/agentflare
Length of output: 13430
🏁 Script executed:
rg -n -C4 'struct ArtifactStore|fn with_store|ArtifactStore::with_store|with_store\(' crates srcRepository: getappz/agentflare
Length of output: 12493
🏁 Script executed:
rg -n -C3 '\bfor_test\s*\(' srcRepository: getappz/agentflare
Length of output: 3136
Use self.store_override when opening the artifact-backed store. The None branch still calls crate::store::open(), so an instance with a private store_override writes artifacts into the default ~/.agentflare/store.db. Reuse the same override path here so private instances stay isolated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server.rs` around lines 419 - 426, Update the artifact store
initialization in the shown match block to use the instance’s
self.store_override when opening the store instead of always calling
crate::store::open(). Preserve the existing fallback to the flat-file
ArtifactStore when opening the selected override fails, ensuring private
instances remain isolated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/coaching/rule.rs (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKnown-host list duplicated with
ALL_HOSTSinsrc/coaching/cli.rs.
KNOWN_SYNC_HOSTShere andALL_HOSTSinsrc/coaching/cli.rsare two independent copies of the same 7 host strings. If a host is ever added/removed from one but not the other,--syncvalidation andcoaching sync --agentwill silently disagree on what's "known".Consider exporting one list (e.g.
pub(super) const KNOWN_SYNC_HOSTS) and havingcli.rsreuse it instead of maintainingALL_HOSTSseparately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coaching/rule.rs` around lines 63 - 72, Centralize the host definitions by exporting rule.rs’s KNOWN_SYNC_HOSTS and updating cli.rs to reuse it instead of maintaining the duplicate ALL_HOSTS list. Ensure both --sync validation and coaching sync --agent derive their known-host behavior from this single shared constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components.rs`:
- Around line 255-274: Implement full reconciliation across
src/components.rs:255-274 in sync_now by replacing the exact is_stale_rule
heuristic with provenance-aware drift detection for previously generated
joined-host files and synced Override-tier rules; update
src/components.rs:276-298 in unsync_host to regenerate joined-host files rather
than deleting nonexistent per-rule files; update src/init.rs:434-520 in
wire_opencode_instructions so retain() removes every rules_dir-scoped entry
absent from expected_filenames, not only the hardcoded legacy entry; and update
src/coaching/cli.rs:52-86 in cli_apply to load the prior sync list before
store::apply_rule, then invoke unsync_host for hosts removed by the new --sync
selection.
---
Nitpick comments:
In `@src/coaching/rule.rs`:
- Around line 63-72: Centralize the host definitions by exporting rule.rs’s
KNOWN_SYNC_HOSTS and updating cli.rs to reuse it instead of maintaining the
duplicate ALL_HOSTS list. Ensure both --sync validation and coaching sync
--agent derive their known-host behavior from this single shared constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d896c90-582a-4401-a420-89ff3629a9e7
📒 Files selected for processing (8)
src/cli/coaching.rssrc/coaching/cli.rssrc/coaching/mod.rssrc/coaching/rule.rssrc/coaching/store.rssrc/components.rssrc/hook.rssrc/init.rs
Comment on lines
+255
to
+274
| pub(crate) fn sync_now(host: &str) -> Result<String, String> { | ||
| let mut written = 0usize; | ||
| let mut refreshed = 0usize; | ||
| for (path, content) in rule_targets(host) { | ||
| if !path.exists() { | ||
| if let Some(parent) = path.parent() { | ||
| let _ = fs::create_dir_all(parent); | ||
| } | ||
| fs::write(&path, format!("{content}\n")).map_err(|e| e.to_string())?; | ||
| written += 1; | ||
| } else if crate::init::is_stale_rule(&path, &content) { | ||
| fs::write(&path, format!("{content}\n")).map_err(|e| e.to_string())?; | ||
| refreshed += 1; | ||
| } | ||
| } | ||
| if host == "opencode" { | ||
| crate::init::wire_opencode_instructions(); | ||
| } | ||
| Ok(format!("{written} written, {refreshed} refreshed")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Coaching-to-host sync/unsync only implements "create a new file"; update and removal are all broken for the same underlying reason.
None of these sites implement real reconciliation for already-materialized host content — each only handles the "file doesn't exist yet" case, so updates and removals for joined-host files and already-synced rules are silently dropped.
src/components.rs#L255-L274:sync_now'sis_stale_rule-based refresh only fires on an exact match to a pre-registered "old wording" (compiledrule_text::superseded()or a Builtin-tiersnapshot_previous_body); it never detects real drift for joined-host files (fixed names unrelated to any rule id) or for Override-tier body edits to already-synced rules. Replace the exact-match heuristic with a way to recognize "we generated this before and it's now different" regardless of whether that exact combination was ever registered.src/components.rs#L276-L298:unsync_hostlooks for a standalone{rule_id}.mdthat is never created for joined hosts (cursor/codex/windsurf/vscode-copilot/cline), so it's a guaranteed no-op there — route joined-host teardown through a regenerate-the-joined-file path instead of a per-rule file delete.src/init.rs#L434-L520:wire_opencode_instructionsonly ever adds missinginstructionsentries (plus one hardcoded legacy-engram removal); generalize the retain() cleanup to drop anyrules_dir-scoped entry not in the currentexpected_filenamesset.src/coaching/cli.rs#L52-L86:cli_applynever diffs the rule's previoussynclist against the new one, so hosts dropped via--syncare never unsynced at all — fetch the old sync list before callingstore::apply_ruleand call the (fixed) unsync path for hosts that fell out.
📍 Affects 3 files
src/components.rs#L255-L274(this comment)src/components.rs#L276-L298src/init.rs#L434-L520src/coaching/cli.rs#L52-L86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components.rs` around lines 255 - 274, Implement full reconciliation
across src/components.rs:255-274 in sync_now by replacing the exact
is_stale_rule heuristic with provenance-aware drift detection for previously
generated joined-host files and synced Override-tier rules; update
src/components.rs:276-298 in unsync_host to regenerate joined-host files rather
than deleting nonexistent per-rule files; update src/init.rs:434-520 in
wire_opencode_instructions so retain() removes every rules_dir-scoped entry
absent from expected_filenames, not only the hardcoded legacy entry; and update
src/coaching/cli.rs:52-86 in cli_apply to load the prior sync list before
store::apply_rule, then invoke unsync_host for hosts removed by the new --sync
selection.
getappz
added a commit
that referenced
this pull request
Jul 25, 2026
Fixes the fmt and clippy failures on PR #335, introduced by the coaching tier/sync commits added after the PR's original CI pass: - cargo fmt --all across the files rustfmt flagged (hook_redirect.rs, coaching/{cli,mod,rule,store}.rs, cli/coaching.rs, components.rs, dashboard/artifacts.rs, init.rs, mcp_server.rs, agentflare-artifacts/store.rs) - coaching/cli.rs: surface CoachingRule's tier and applied_at fields in `agentflare coaching list` output (was clippy dead_code; also useful to a user checking whether a rule is builtin vs override, and when it was applied) - coaching/store.rs: drop needless borrow in create_dir_all(rules_dir()) - init.rs: use Vec::contains instead of iter().any() for a plain equality check
ArtifactStore gains with_store(Store) constructor backed by documents + blobs (content-addressed, gzip, deduped). Metadata serialized to documents.metadata JSON. Version history via doc_history. FTS5 search replaces flat-file scan (fixes #180). Dashboard serves artifacts under /artifacts/ via axum routes (index, artifact page, version page, versions JSON, SSE live). No more random-port URL — uses dashboard port. Integration: ensure_artifact_server, CLI handoff, standalone serve all default to store backend, fall back to flat-file. 32 tests pass (26 original + 6 new store-backed).
… files opencode ran an rm mid-migration and silently wiped store.db's metadata, recovered by hand from a pre-migration flat-file backup that happened to still exist. Extend the shared PreToolUse classifier to deny destructive shell commands targeting agentflare's own db files or the data dir itself, for Bash and PowerShell tool calls.
DocUpsertOpts gained a track_history field upstream since this branch's artifact-store migration was written; doc_upsert_with_opts's history-skip check compared old_content != content, but blob-backed callers (artifacts) always pass an empty content string and store the real payload via blob_hash, so the check was always a no-op false and history rows never got recorded. Compare blob_hash too. Also fixes two clippy findings (io_other_error, needless_borrow) surfaced by -D warnings.
…n, staleness, CLI RuleTier (Builtin/Override), sync fields on CoachingRule, parse/write. apply_rule/remove_rule carry tier+sync, sync_targets_for_host query. rule_targets merges coaching rules per host (per-file or joined). Snapshot previous body on builtin overwrite; is_stale_rule checks it. sync_now/unsync_host for immediate materialization. CLI: --tier/--sync flags, sync subcommand. Tested: 112 pass across coaching/components/init/hook.
wire_opencode_instructions's doc comment promised removing entries for rules no longer synced, but only the hardcoded legacy engram.md path was ever pruned -- unsync_host deleting a coaching rule's file left a dangling reference to it in opencode.jsonc forever. Retain only array entries under our rules_dir whose filename is still expected.
Fixes the fmt and clippy failures on PR #335, introduced by the coaching tier/sync commits added after the PR's original CI pass: - cargo fmt --all across the files rustfmt flagged (hook_redirect.rs, coaching/{cli,mod,rule,store}.rs, cli/coaching.rs, components.rs, dashboard/artifacts.rs, init.rs, mcp_server.rs, agentflare-artifacts/store.rs) - coaching/cli.rs: surface CoachingRule's tier and applied_at fields in `agentflare coaching list` output (was clippy dead_code; also useful to a user checking whether a rule is builtin vs override, and when it was applied) - coaching/store.rs: drop needless borrow in create_dir_all(rules_dir()) - init.rs: use Vec::contains instead of iter().any() for a plain equality check
getappz
force-pushed
the
store-and-db-guard
branch
from
July 26, 2026 00:49
f779a84 to
b8bb87c
Compare
4 tasks
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
with_store(Store)backend: content-addressed blobs, gzip, dedup, FTS5 search, version history viadoc_history; dashboard serves artifacts under/artifacts/; falls back to flat-file storage on error)*.db/*.db-wal/*.db-shmfiles or the.agentflaredir itself — landed after opencode ran anrmmid-migration today and silently wipedstore.db's metadata for 168 artifacts, recovered by hand from a pre-migration flat-file backup that happened to still existdoc_upsert_with_opts's history-skip check, which never recorded history for blob-backed callers (compared an always-emptycontentstring instead of also checkingblob_hash) — surfaced by running the existing artifact-store test suite against current masterio_other_error,needless_borrow) surfaced by-D warningsTest plan
cargo test --bin agentflare hook_redirect— 31/31 pass, including new regression tests for the delete-guard and a false-positive case (agit commitheredoc whose message describes the incident in prose must not itself get blocked)cargo test -p agentflare-store— 46/46 passcargo test -p agentflare-artifacts— 32/32 passcargo clippy --workspace --all-features -- -D warnings— cleancargo test --workspace— 796/799 pass; 3 pre-existing failures (state::tests::*,vent::capture::tests::append_routed_suppresses_second_identical_call_within_window) confirmed flaky/unrelated — pass individually in isolation, don't touch anything in this diffSummary by CodeRabbit