Conversation
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.
…d resolve_trust_root_touch
|
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:
📝 WalkthroughWalkthroughThe PR adds document/blob-backed artifact storage and dashboard routes, layered git-shim policy configuration, coaching rule tiers with host synchronization, stale-rule cleanup, and shell protection against destructive agentflare data deletion. ChangesArtifact storage and dashboard
Configurable git-shim policy
Coaching rule tiers and synchronization
Destructive data-file command blocking
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/agentflare-store/src/documents.rs (1)
185-209: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
unchangedsuppresses the history row but not the version bump.A no-op re-upsert (both
contentandblob_hashidentical) now skips thestore_doc_historyinsert, yet Line 203 still setsversion = old_version + 1. The document's version therefore advances with no history row for the version it left behind, sodoc_historyacquires holes andget_version(id, N)for a skipped N cannot resolve.
agentflare-artifactsmasks this by short-circuiting the whole upsert when unchanged, but any other caller (doc_upsert, cache-style writers) hits it directly. Either bump the version only when changed, or record history unconditionally.🛠️ Suggested fix
let unchanged = old_content == content && old_blob_hash == opts.blob_hash; - let new_version = old_version + 1; + let new_version = if unchanged { old_version } else { old_version + 1 };(move the
new_versionbinding belowunchanged; Line 183 currently precedes it)Note also that
blob_hashis only written back whenopts.blob_hash.is_some()(Line 224), so a caller passingNoneagainst a blob-backed row computesunchanged == falseevery time and snapshots history on each call.🤖 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 185 - 209, Update the upsert versioning flow around unchanged and new_version so a no-op re-upsert does not increment the document version when both content and blob_hash are unchanged; compute the next version only for changed updates, while preserving history insertion for genuine changes. Also ensure blob-backed rows compare the effective existing blob hash when opts.blob_hash is None, so omitted blob values do not trigger repeated false changes.
🧹 Nitpick comments (6)
crates/agentflare-artifacts/src/store.rs (2)
483-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_storeduplicatesdoc_to_artifactfield-for-field.The only difference is
content. Reuse the helper so the mapping has one source of truth (the same shapeget_version_storealready uses at Line 433).♻️ Proposed refactor
let content = Self::blob_or_content(store, &doc)?; - let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) - .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; - Ok(Artifact { - id: doc.path.clone(), - name: doc.title.clone(), - artifact_type: meta.artifact_type, - content, - session_id: doc.session_id.clone().unwrap_or_default(), - created_at: doc.created_at as u64, - updated_at: doc.updated_at as u64, - version: doc.version as u32, - description: meta.description, - favicon: meta.favicon, - sender: meta.sender, - recipient: meta.recipient, - thread_id: meta.thread_id, - reply_to: meta.reply_to, - git: meta.git, - }) + Ok(Artifact { + content, + ..Self::doc_to_artifact(&doc)? + })🤖 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 483 - 513, Update get_store to reuse the existing doc_to_artifact helper for all Artifact field mapping, passing the content obtained from blob_or_content as required. Remove the duplicated metadata parsing and field-by-field construction while preserving the current lookup, not-found, and content-loading behavior.
611-619: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffDeleted artifacts leave their blobs behind.
doc_deleteis a soft delete on the document row; the content blobs referenced by the live row and every history row stay in the blob table forever. With versioned artifacts this is the dominant on-disk cost. Consider a reference-counted blob GC (or a periodic sweep for blob hashes unreferenced by any document/history row) before this becomes the default backend everywhere.🤖 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 611 - 619, Update the artifact deletion flow around the visible store.doc_delete call so deleting an artifact also removes or garbage-collects its content blobs, including blobs referenced by the live document and all history rows. Implement reference-aware cleanup (or an equivalent sweep of unreferenced blob hashes) that preserves blobs still referenced by other document/history rows, and only report successful deletion after the cleanup completes.src/dashboard/server.rs (1)
230-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
router()now opens the real store as a construction side effect.
super::artifacts::router()callsopen_store()eagerly, so merely building the dashboard router opens (and creates)~/.agentflare's store — including inclaims_endpoint_returns_json_arrayat Line 257, which previously touched no artifact state. Consider constructing theArtifactStatelazily (e.g.OnceLock/Lazyinside the handlers) or letting the caller inject it, so router construction stays side-effect free and testable.🤖 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/server.rs` at line 230, Update super::artifacts::router() and its ArtifactState initialization so building the dashboard router does not eagerly call open_store() or create artifact state. Lazily initialize the store within the artifact handlers using the existing state flow, or inject a prebuilt state from the caller, while preserving handler behavior and keeping router construction side-effect free.src/cli/handoff.rs (1)
92-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the store-open + flat-file fallback into one helper. Both sites open the unified store, wrap it with
ArtifactStore::with_store, and on error log and fall back toArtifactStore::new(home/.agentflare/artifacts)— the fallback directory and the log prefix are already duplicated literals, and the wording has drifted between them. A singlecrate::store::artifact_store()returningArtifactStorekeeps the fallback path, the message, and any future change (retry, one-shot warning, metrics) in one place.
src/cli/handoff.rs#L92-L103: replace the innermatch crate::store::open()arm with a call to the shared helper, keeping theself.dirbranch as-is.src/dashboard/artifacts.rs#L20-L32: replace thematch crate::store::open()block inopen_storewith the shared helper, dropping the divergent[dashboard/artifacts] failed to open storewording.A third copy exists in
src/artifacts.rs(outside this review's file set) and should adopt the same helper.🤖 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/cli/handoff.rs` around lines 92 - 103, Extract the shared store-opening and flat-file fallback logic into crate::store::artifact_store(), including the unified fallback path and log message. In src/cli/handoff.rs lines 92-103, replace the inner crate::store::open() match with this helper while keeping the self.dir branch unchanged; in src/dashboard/artifacts.rs lines 20-32, replace the open_store match with the helper and remove its divergent log wording. Also update the duplicate logic in src/artifacts.rs to use the same helper.src/coaching/rule.rs (2)
210-220: 📐 Maintainability & Code Quality | 🔵 TrivialConsider warning on malformed
# Tier:like# Trigger:does.A malformed/unknown
# Tier:value silently falls back toRuleTier::Overridewith no diagnostic, whereas an unparseable# Trigger:line emits aneprintln!warning just above. For consistency and easier debugging of hand-edited or corrupted rule files, consider logging similarly whenRuleTier::parsereturnsNone.♻️ Suggested tweak
} else if let Some(rest) = line.strip_prefix("# Tier:") { if let Some(t) = RuleTier::parse(rest) { tier = t; + } else { + eprintln!( + "[agentflare] coaching: unknown Tier value, defaulting to override: {rest:?}" + ); } } else if let Some(rest) = line.strip_prefix("# Sync:") {🤖 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 210 - 220, Update the `# Tier:` parsing branch in the rule-loading logic to emit an `eprintln!` warning when `RuleTier::parse(rest)` returns `None`, matching the diagnostic behavior of the nearby `# Trigger:` handling. Preserve the existing tier assignment for valid values and the default `RuleTier::Override` behavior for malformed values.
63-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate host allowlists — extract a single shared list.
KNOWN_SYNC_HOSTS(validation) andALL_HOSTS(default sync-all target set) enumerate the exact same 7 hosts in two separate files with no shared definition; a future host addition/removal to one list without updating the other would silently desync what's validated from what's synced by default.
src/coaching/rule.rs#L63-L71: keepKNOWN_SYNC_HOSTSas the single source of truth (e.g. make itpub(super)/pub(crate)).src/coaching/cli.rs#L6-L14: drop the separateALL_HOSTSconstant and reuserule::KNOWN_SYNC_HOSTSforcli_sync's default target set.🤖 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 - 71, Make rule::KNOWN_SYNC_HOSTS the shared source of truth by exposing it to the coaching CLI module; in src/coaching/rule.rs lines 63-71, adjust its visibility without changing the host entries. In src/coaching/cli.rs lines 6-14, remove ALL_HOSTS and update cli_sync’s default target set to reuse rule::KNOWN_SYNC_HOSTS.
🤖 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-artifacts/src/store.rs`:
- Around line 138-171: Update meta_to_metadata to accept the artifact id and
assign it to ArtifactMeta.id instead of String::new(). Modify its call site in
the store publish flow to pass the real id through, preserving that id in
persisted metadata for all readers.
- Around line 185-192: Align update_id handling in the publish path around the
visible id assignment and doc_get_by_path flow with publish_flat: accept the
requested update_id only when artifact_live validates it and
self.artifact_dir(uid).exists(); otherwise generate a fresh nanoid. Ensure stale
or arbitrary ids cannot become document paths, while preserving valid
existing-id updates.
In `@crates/flare-git-core/src/config_loader.rs`:
- Around line 17-27: Update parse_if_exists so only a not-found read_to_string
error returns Ok(None); propagate other IO failures as LoaderError with the
config path and underlying error, preserving the existing TOML parse error
handling.
- Around line 9-15: Update the LoaderError thiserror format attribute to render
the path through its Display-compatible representation, such as path.display(),
while preserving the existing source error formatting.
In `@crates/flare-git-core/src/policy_config.rs`:
- Around line 63-104: Update resolve so project_local cannot relax baseline
denied plumbing protections: exclude
project_local.extra_allowed_mutating_subcommands from
allowed_mutating_subcommands, or otherwise prevent entries overlapping
denied_plumbing_subcommands from taking effect, while retaining user_home
relaxation. Preserve project-local handling for trust roots and other
non-conflicting policy settings.
In `@src/artifacts.rs`:
- Around line 12-13: Update the path passed to ArtifactStore::new in the
artifact store initialization to use the store root (~/.agentflare) rather than
appending the artifacts directory; ArtifactStore::new already adds
ARTIFACTS_DIR, so preserve the resulting flat ~/.agentflare/artifacts location.
In `@src/coaching/cli.rs`:
- Around line 101-117: Update cli_remove’s per-host unsync_host loop to log each
failure without calling std::process::exit(1), allowing cleanup to continue for
every host in sync. Preserve the final success message and existing
store::remove_rule error handling.
In `@src/coaching/store.rs`:
- Around line 110-134: Update the overwrite snapshot condition in the
rule-writing flow to snapshot changed existing rules for the default Override
tier as well as Builtin, while preserving the existing checks for is_overwrite,
matching rule ID, and changed body. Ensure snapshot_previous_body runs before
rule::write_rule_file so subsequent is_stale_rule and sync_now calls detect the
replacement.
In `@src/dashboard/artifacts.rs`:
- Around line 53-56: Update the artifact_page and artifact_version_page
handlers, along with the versions access around the referenced ranges, to move
synchronous store calls (get, get_version, and versions) and artifact HTML
rendering into spawn_blocking, following the existing index pattern. Preserve
the current not-found responses and rendered response behavior while ensuring no
rusqlite, filesystem/blob reads, or render_artifact_page calls execute directly
on the async runtime thread.
- Around line 49-83: Apply the existing agentflare_artifacts::valid_id guard
used by artifact_live to the id parameter in artifact_page,
artifact_version_page, and versions_json, returning the same not-found response
before calling the store when validation fails. Preserve the existing valid-ID
lookup and response behavior.
- Around line 92-106: Update the SSE subscription flow around
ArtifactStore::subscribe and the spawn_blocking loop to avoid permanently
occupied blocking threads, preferably by using tokio::sync::broadcast and an
async receiver; otherwise poll with recv_timeout and exit when the client
channel closes. Ensure abandoned subscriptions are removed from live_broadcast
when the SSE client disconnects, and configure Sse::new(stream) with
keep_alive(Default::default()) so idle connections remain active.
In `@src/hook_redirect.rs`:
- Around line 89-95: Update the destructive-command detection around
is_destructive_verb so it examines the effective command rather than only
first_word. Unwrap common launchers such as sudo and env, and inspect nested
shell commands passed to bash or equivalent, so commands like sudo rm, env rm,
and bash -c 'rm ...' are recognized while preserving the existing destructive
verb list and guard behavior.
- Around line 79-84: Update the statement-splitting loop in the hook command
parsing flow to use quote- and escape-aware tokenization, so separators inside
quoted arguments remain part of the same command while unquoted shell operators
still split statements. Preserve the existing safety checks and add a regression
test covering a quoted git commit message containing a separator and
destructive-looking text.
- Around line 96-107: Normalize the command target in the redirect logic before
evaluating targets_agentflare_dir and targets_db_or_whole_dir: strip surrounding
quotes and trailing slash or backslash separators. Ensure quoted or
slash-terminated .agentflare paths are recognized as the whole protected
directory and denied, while retaining existing file and recursive-delete
detection.
- Around line 197-202: Update the command extraction logic in the hook redirect
path to select the first alias whose value is a string, rather than stopping at
a present but non-string command value; preserve the alias order command, cmd,
then script. Add a regression test covering a non-string command alongside a
valid cmd or script and verify the valid alias is processed for blocking.
In `@src/init.rs`:
- Around line 494-521: Update the pruning and deduplication logic around
expected_filenames to compare exact path basenames rather than filename
substrings. In the retain closure, extract each entry’s basename and match it
exactly against the expected filename; in the has_it check, likewise compare
basenames exactly for both arr entries and sibling_instructions, preserving the
existing behavior for non-path values.
In `@src/mcp_server.rs`:
- Around line 419-426: Update the artifact-store initialization in
ensure_artifact_server to honor the AgentflareMcp store_override instead of
unconditionally calling crate::store::open(). Reuse the existing override-aware
store-opening path used elsewhere in the file, while preserving the current
fallback to the flat-file ArtifactStore when opening fails.
---
Outside diff comments:
In `@crates/agentflare-store/src/documents.rs`:
- Around line 185-209: Update the upsert versioning flow around unchanged and
new_version so a no-op re-upsert does not increment the document version when
both content and blob_hash are unchanged; compute the next version only for
changed updates, while preserving history insertion for genuine changes. Also
ensure blob-backed rows compare the effective existing blob hash when
opts.blob_hash is None, so omitted blob values do not trigger repeated false
changes.
---
Nitpick comments:
In `@crates/agentflare-artifacts/src/store.rs`:
- Around line 483-513: Update get_store to reuse the existing doc_to_artifact
helper for all Artifact field mapping, passing the content obtained from
blob_or_content as required. Remove the duplicated metadata parsing and
field-by-field construction while preserving the current lookup, not-found, and
content-loading behavior.
- Around line 611-619: Update the artifact deletion flow around the visible
store.doc_delete call so deleting an artifact also removes or garbage-collects
its content blobs, including blobs referenced by the live document and all
history rows. Implement reference-aware cleanup (or an equivalent sweep of
unreferenced blob hashes) that preserves blobs still referenced by other
document/history rows, and only report successful deletion after the cleanup
completes.
In `@src/cli/handoff.rs`:
- Around line 92-103: Extract the shared store-opening and flat-file fallback
logic into crate::store::artifact_store(), including the unified fallback path
and log message. In src/cli/handoff.rs lines 92-103, replace the inner
crate::store::open() match with this helper while keeping the self.dir branch
unchanged; in src/dashboard/artifacts.rs lines 20-32, replace the open_store
match with the helper and remove its divergent log wording. Also update the
duplicate logic in src/artifacts.rs to use the same helper.
In `@src/coaching/rule.rs`:
- Around line 210-220: Update the `# Tier:` parsing branch in the rule-loading
logic to emit an `eprintln!` warning when `RuleTier::parse(rest)` returns
`None`, matching the diagnostic behavior of the nearby `# Trigger:` handling.
Preserve the existing tier assignment for valid values and the default
`RuleTier::Override` behavior for malformed values.
- Around line 63-71: Make rule::KNOWN_SYNC_HOSTS the shared source of truth by
exposing it to the coaching CLI module; in src/coaching/rule.rs lines 63-71,
adjust its visibility without changing the host entries. In src/coaching/cli.rs
lines 6-14, remove ALL_HOSTS and update cli_sync’s default target set to reuse
rule::KNOWN_SYNC_HOSTS.
In `@src/dashboard/server.rs`:
- Line 230: Update super::artifacts::router() and its ArtifactState
initialization so building the dashboard router does not eagerly call
open_store() or create artifact state. Lazily initialize the store within the
artifact handlers using the existing state flow, or inject a prebuilt state from
the caller, while preserving handler behavior and keeping router construction
side-effect free.
🪄 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: 23b56ce0-9688-42e5-bb7f-657a3e34a67a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
crates/agentflare-artifacts/Cargo.tomlcrates/agentflare-artifacts/src/store.rscrates/agentflare-store/src/documents.rscrates/flare-git-core/Cargo.tomlcrates/flare-git-core/src/classify.rscrates/flare-git-core/src/config_loader.rscrates/flare-git-core/src/lib.rscrates/flare-git-core/src/policy_config.rssrc/artifacts.rssrc/cli/coaching.rssrc/cli/handoff.rssrc/coaching/cli.rssrc/coaching/mod.rssrc/coaching/rule.rssrc/coaching/store.rssrc/components.rssrc/dashboard/artifacts.rssrc/dashboard/mod.rssrc/dashboard/server.rssrc/hook.rssrc/hook_redirect.rssrc/init.rssrc/mcp_server.rs
| fn meta_to_metadata( | ||
| prev: Option<&ArtifactMeta>, | ||
| req: &PublishRequest, | ||
| new_version: u32, | ||
| now: i64, | ||
| ) -> String { | ||
| let keep = |new: &Option<String>, old: fn(&ArtifactMeta) -> Option<String>| { | ||
| new.clone().or_else(|| prev.and_then(old)) | ||
| }; | ||
| let mut history = prev.map(|m| m.history.clone()).unwrap_or_default(); | ||
| history.push(VersionInfo { | ||
| version: new_version, | ||
| label: req.label.clone(), | ||
| created_at: now as u64, | ||
| }); | ||
| let meta = ArtifactMeta { | ||
| id: String::new(), | ||
| name: req.name.clone(), | ||
| artifact_type: req.artifact_type.clone(), | ||
| session_id: req.session_id.clone(), | ||
| created_at: prev.map(|m| m.created_at).unwrap_or(now as u64), | ||
| updated_at: now as u64, | ||
| version: new_version, | ||
| description: keep(&req.description, |m| m.description.clone()), | ||
| favicon: keep(&req.favicon, |m| m.favicon.clone()), | ||
| history, | ||
| sender: keep(&req.sender, |m| m.sender.clone()), | ||
| recipient: keep(&req.recipient, |m| m.recipient.clone()), | ||
| thread_id: keep(&req.thread_id, |m| m.thread_id.clone()), | ||
| reply_to: keep(&req.reply_to, |m| m.reply_to.clone()), | ||
| git: req.git.clone().or_else(|| prev.and_then(|m| m.git.clone())), | ||
| }; | ||
| serde_json::to_string(&meta).unwrap_or_else(|_| "{}".into()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Persist the real artifact id in the document metadata.
meta_to_metadata writes id: String::new(), so every store-backed artifact's persisted metadata JSON carries "id": "". Store-mode readers happen to take the id from doc.path, but the flat-file reader (get, list) reads meta.id — any export/migration or a future reader of this blob gets an empty id. Pass the id through.
🛠️ Proposed fix
fn meta_to_metadata(
+ id: &str,
prev: Option<&ArtifactMeta>,
req: &PublishRequest,
new_version: u32,
now: i64,
) -> String {
@@
let meta = ArtifactMeta {
- id: String::new(),
+ id: id.to_string(),Update the call site at line 226 accordingly.
🤖 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 138 - 171, Update
meta_to_metadata to accept the artifact id and assign it to ArtifactMeta.id
instead of String::new(). Modify its call site in the store publish flow to pass
the real id through, preserving that id in persisted metadata for all readers.
| let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!()); | ||
| let now = db_kit::ids::now(); | ||
| let path = &id; | ||
|
|
||
| // Read existing doc to check CAS and dedup | ||
| let existing = store | ||
| .doc_get_by_path(DOC_PROJECT, path) | ||
| .map_err(Self::store_conn_err)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
update_id handling diverges from the flat path and is unvalidated.
publish_flat filters update_id through self.artifact_dir(uid).exists(), so an id that doesn't resolve to an existing artifact yields a freshly minted nanoid. Here the raw update_id is used unconditionally as the document path, so a stale/unknown id silently creates a new artifact under a caller-supplied key instead of returning a new id. Given artifact_live already gates on agentflare_artifacts::valid_id, the write path accepting arbitrary strings as document paths is the looser end.
🛠️ Suggested alignment
- let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!());
+ let id = req
+ .update_id
+ .clone()
+ .filter(|uid| valid_id(uid))
+ .unwrap_or_else(|| nanoid::nanoid!());📝 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.
| let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!()); | |
| let now = db_kit::ids::now(); | |
| let path = &id; | |
| // Read existing doc to check CAS and dedup | |
| let existing = store | |
| .doc_get_by_path(DOC_PROJECT, path) | |
| .map_err(Self::store_conn_err)?; | |
| let id = req | |
| .update_id | |
| .clone() | |
| .filter(|uid| valid_id(uid)) | |
| .unwrap_or_else(|| nanoid::nanoid!()); | |
| let now = db_kit::ids::now(); | |
| let path = &id; | |
| // Read existing doc to check CAS and dedup | |
| let existing = store | |
| .doc_get_by_path(DOC_PROJECT, path) | |
| .map_err(Self::store_conn_err)?; |
🤖 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 185 - 192, Align
update_id handling in the publish path around the visible id assignment and
doc_get_by_path flow with publish_flat: accept the requested update_id only when
artifact_live validates it and self.artifact_dir(uid).exists(); otherwise
generate a fresh nanoid. Ensure stale or arbitrary ids cannot become document
paths, while preserving valid existing-id updates.
| #[derive(Debug, thiserror::Error)] | ||
| #[error("{path}: {source}")] | ||
| pub struct LoaderError { | ||
| pub path: PathBuf, | ||
| #[source] | ||
| pub source: toml::de::Error, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does thiserror 2.x automatically call .display() or Debug for PathBuf fields in #[error(...)] format strings?
💡 Result:
No, thiserror does not automatically call.display for PathBuf fields when used in #[error(...)] format strings [1][2]. Because PathBuf does not implement the standard library's Display trait, using it directly in an error message format string (e.g., #[error("failed at {path}")]) will cause a compilation error [1][2]. To format a PathBuf in an error message, you must manually call.display (e.g., #[error("failed at {}", path.display)]) [3][4]. While thiserror includes internal helper traits (such as AsDisplay) to facilitate some formatting operations when the std feature is enabled, these do not automatically transform PathBuf fields for the standard format macros used within the #[error] attribute [5][6]. You must explicitly invoke the display method provided by the standard library to convert the path into a type that implements Display [3][1][2].
Citations:
- 1: Support Display and Debug of same path in error message dtolnay/thiserror#383
- 2: Associated type equality constraints interfere with display conversion dtolnay/thiserror#335
- 3: https://github.com/dtolnay/thiserror/blob/master/Cargo.toml
- 4: https://projectlighthouse.io/en/books/rust-from-zero/pages/file-io-and-error-handling
- 5: https://codebrowser.dev/rust/crates/thiserror-2.0.11/src/display.rs.html
- 6: https://docs.rs/crate/thiserror/latest/source/src/display.rs
🏁 Script executed:
sed -n '1,120p' crates/flare-git-core/src/config_loader.rsRepository: getappz/agentflare
Length of output: 3541
LoaderError won’t compile as written: PathBuf doesn’t implement Display. #[error("{path}: {source}")] needs path.display() (or another Display expression) instead of bare {path}.
Proposed fix
#[derive(Debug, thiserror::Error)]
-#[error("{path}: {source}")]
+#[error("{}: {source}", path.display())]
pub struct LoaderError {
pub path: PathBuf,
#[source]
pub source: toml::de::Error,
}📝 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.
| #[derive(Debug, thiserror::Error)] | |
| #[error("{path}: {source}")] | |
| pub struct LoaderError { | |
| pub path: PathBuf, | |
| #[source] | |
| pub source: toml::de::Error, | |
| } | |
| #[derive(Debug, thiserror::Error)] | |
| #[error("{}: {source}", path.display())] | |
| pub struct LoaderError { | |
| pub path: PathBuf, | |
| #[source] | |
| pub source: toml::de::Error, | |
| } |
🤖 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/flare-git-core/src/config_loader.rs` around lines 9 - 15, Update the
LoaderError thiserror format attribute to render the path through its
Display-compatible representation, such as path.display(), while preserving the
existing source error formatting.
| fn parse_if_exists(path: &Path) -> Result<Option<(PathBuf, toml::Value)>, LoaderError> { | ||
| let Ok(contents) = std::fs::read_to_string(path) else { | ||
| return Ok(None); | ||
| }; | ||
| toml::from_str(&contents) | ||
| .map(|v| Some((path.to_path_buf(), v))) | ||
| .map_err(|source| LoaderError { | ||
| path: path.to_path_buf(), | ||
| source, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Permission/IO errors on the config file are silently treated as "no config."
parse_if_exists collapses every read_to_string failure (not just "file doesn't exist") into Ok(None). A misconfigured permission bit on config.toml silently falls back to baseline with no warning, unlike the malformed-TOML path which does warn — undermining the fail-safe-with-warning design goal described in the PR.
🛠️ Suggested fix: distinguish "not found" from other IO errors
fn parse_if_exists(path: &Path) -> Result<Option<(PathBuf, toml::Value)>, LoaderError> {
- let Ok(contents) = std::fs::read_to_string(path) else {
- return Ok(None);
- };
+ let contents = match std::fs::read_to_string(path) {
+ Ok(c) => c,
+ Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None),
+ Err(e) => {
+ eprintln!("WARNING: could not read {} ({e}) -- treating as absent", path.display());
+ return Ok(None);
+ }
+ };
toml::from_str(&contents)📝 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.
| fn parse_if_exists(path: &Path) -> Result<Option<(PathBuf, toml::Value)>, LoaderError> { | |
| let Ok(contents) = std::fs::read_to_string(path) else { | |
| return Ok(None); | |
| }; | |
| toml::from_str(&contents) | |
| .map(|v| Some((path.to_path_buf(), v))) | |
| .map_err(|source| LoaderError { | |
| path: path.to_path_buf(), | |
| source, | |
| }) | |
| } | |
| fn parse_if_exists(path: &Path) -> Result<Option<(PathBuf, toml::Value)>, LoaderError> { | |
| let contents = match std::fs::read_to_string(path) { | |
| Ok(c) => c, | |
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), | |
| Err(e) => { | |
| eprintln!("WARNING: could not read {} ({e}) -- treating as absent", path.display()); | |
| return Ok(None); | |
| } | |
| }; | |
| toml::from_str(&contents) | |
| .map(|v| Some((path.to_path_buf(), v))) | |
| .map_err(|source| LoaderError { | |
| path: path.to_path_buf(), | |
| source, | |
| }) | |
| } |
🤖 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/flare-git-core/src/config_loader.rs` around lines 17 - 27, Update
parse_if_exists so only a not-found read_to_string error returns Ok(None);
propagate other IO failures as LoaderError with the config path and underlying
error, preserving the existing TOML parse error handling.
| fn parse_git_shim(layer: Option<(PathBuf, toml::Value)>) -> Result<GitShimConfig, LoaderError> { | ||
| let Some((path, value)) = layer else { | ||
| return Ok(GitShimConfig::default()); | ||
| }; | ||
| ConfigFile::deserialize(value) | ||
| .map(|f| f.git_shim) | ||
| .map_err(|source| LoaderError { path, source }) | ||
| } | ||
|
|
||
| pub fn resolve( | ||
| repo_root: &Path, | ||
| home: Option<&Path>, | ||
| ) -> Result<ResolvedGitShimPolicy, LoaderError> { | ||
| let layers = config_loader::locate_and_parse(repo_root, home)?; | ||
| let project_local = parse_git_shim(layers.project_local)?; | ||
| let user_home = parse_git_shim(layers.user_home)?; | ||
|
|
||
| Ok(ResolvedGitShimPolicy { | ||
| trust_root_paths: unioned( | ||
| TRUST_ROOT_PATHS, | ||
| [ | ||
| &project_local.extra_trust_root_paths, | ||
| &user_home.extra_trust_root_paths, | ||
| &extra_trust_root_paths_from_env(), | ||
| ], | ||
| ), | ||
| allowed_mutating_subcommands: unioned( | ||
| ALLOWED_MUTATING_SUBCOMMANDS, | ||
| [ | ||
| &project_local.extra_allowed_mutating_subcommands, | ||
| &user_home.extra_allowed_mutating_subcommands, | ||
| ], | ||
| ), | ||
| denied_plumbing_subcommands: unioned( | ||
| DENIED_PLUMBING_SUBCOMMANDS, | ||
| [ | ||
| &project_local.extra_denied_plumbing_subcommands, | ||
| &user_home.extra_denied_plumbing_subcommands, | ||
| ], | ||
| ), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Project-local config can silently relax the exact plumbing-command protections it's meant to enforce.
resolve() merges project_local (read from repo_root/.agentflare/config.toml, a plain file inside the working tree) with the same trust level as user_home for extra_allowed_mutating_subcommands. Since classify_pure checks the allow-list before the deny-list, project-local config can relax any baseline-denied plumbing subcommand — exactly the class of command the shim's own deny message calls out as able to "bypass the checks this shim applies to higher-level commands" (confirmed by the project_local_config_can_relax_a_denied_plumbing_subcommand test in classify.rs).
Unlike a git push of .agentflare/config.toml to the default branch (which the trust-root guard blocks), a local, uncommitted edit to this file takes effect on the very next shimmed git command — no commit, push, or review required. Any process with plain filesystem write access to the repo (e.g. the orchestrated coding agent itself) can use this to disable the shim's plumbing-command protections at will, without ever going through git.
Consider not letting project-local config relax denied_plumbing_subcommands (or extra_allowed_mutating_subcommands overlapping with baseline-denied plumbing commands) at all — reserve that relaxation for the user-home layer, which is outside the agent-controlled working tree.
🤖 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/flare-git-core/src/policy_config.rs` around lines 63 - 104, Update
resolve so project_local cannot relax baseline denied plumbing protections:
exclude project_local.extra_allowed_mutating_subcommands from
allowed_mutating_subcommands, or otherwise prevent entries overlapping
denied_plumbing_subcommands from taking effect, while retaining user_home
relaxation. Preserve project-local handling for trust roots and other
non-conflicting policy settings.
| let is_destructive_verb = matches!( | ||
| first_word, | ||
| "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir" | ||
| ); | ||
| if !is_destructive_verb { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not rely solely on the statement’s first word.
Commands such as sudo rm ~/.agentflare/store.db, env rm ~/.agentflare/store.db, or bash -c 'rm ~/.agentflare/store.db' execute the destructive verb but begin with sudo, env, or bash, so they bypass this guard. Unwrap common launchers or parse nested shell commands before deciding.
🤖 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 89 - 95, Update the destructive-command
detection around is_destructive_verb so it examines the effective command rather
than only first_word. Unwrap common launchers such as sudo and env, and inspect
nested shell commands passed to bash or equivalent, so commands like sudo rm,
env rm, and bash -c 'rm ...' are recognized while preserving the existing
destructive verb list and guard behavior.
| 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"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Normalize quoted and slash-terminated directory targets.
rm -rf "$HOME/.agentflare" fails the .agentflare//.agentflare checks because the path ends in a quote. Likewise, rmdir /s /q C:\Users\...\ .agentflare\ becomes .agentflare/, which passes the first check but fails the whole-directory check. Both can delete the protected directory without being denied.
🤖 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 96 - 107, Normalize the command target in
the redirect logic before evaluating targets_agentflare_dir and
targets_db_or_whole_dir: strip surrounding quotes and trailing slash or
backslash separators. Ensure quoted or slash-terminated .agentflare paths are
recognized as the whole protected directory and denied, while retaining existing
file and recursive-delete detection.
| let input = tool_input?; | ||
| let command = input | ||
| .get("command") | ||
| .or_else(|| input.get("cmd")) | ||
| .or_else(|| input.get("script")) | ||
| .and_then(Value::as_str)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make command-alias fallback type-aware.
If command exists but is not a string, .or_else(...) selects it and as_str()? returns None; a valid cmd or script is never checked. This can bypass blocking for mixed or malformed tool payloads. Select the first alias whose value is actually a string, and add a regression test.
Suggested extraction
- let command = input
- .get("command")
- .or_else(|| input.get("cmd"))
- .or_else(|| input.get("script"))
- .and_then(Value::as_str)?;
+ let command = ["command", "cmd", "script"]
+ .iter()
+ .find_map(|key| input.get(*key).and_then(Value::as_str))?;📝 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.
| let input = tool_input?; | |
| let command = input | |
| .get("command") | |
| .or_else(|| input.get("cmd")) | |
| .or_else(|| input.get("script")) | |
| .and_then(Value::as_str)?; | |
| let input = tool_input?; | |
| let command = ["command", "cmd", "script"] | |
| .iter() | |
| .find_map(|key| input.get(*key).and_then(Value::as_str))?; |
🤖 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 197 - 202, Update the command extraction
logic in the hook redirect path to select the first alias whose value is a
string, rather than stopping at a present but non-string command value; preserve
the alias order command, cmd, then script. Add a regression test covering a
non-string command alongside a valid cmd or script and verify the valid alias is
processed for blocking.
| arr.retain(|v| { | ||
| let Some(s) = v.as_str() else { return true }; | ||
| let normalized = s.replace('\\', "/"); | ||
| if !normalized.starts_with(&rules_dir_str) { | ||
| return true; | ||
| } | ||
| expected_filenames | ||
| .iter() | ||
| .any(|f| normalized.ends_with(f.as_str())) | ||
| }); | ||
| let pruned = before_prune - arr.len(); | ||
|
|
||
| let mut added = 0; | ||
| for &file in rule_files { | ||
| for file in &expected_filenames { | ||
| let rule_path = rules_dir.join(file); | ||
| let path_str = rule_path.to_string_lossy().replace('\\', "/"); | ||
| let has_it = arr | ||
| let has_it = arr.iter().any(|v| { | ||
| v.as_str() | ||
| .map(|s| s.contains(file.as_str())) | ||
| .unwrap_or(false) | ||
| }) || sibling_instructions | ||
| .iter() | ||
| .any(|v| v.as_str().map(|s| s.contains(file)).unwrap_or(false)) | ||
| || sibling_instructions.iter().any(|s| s.contains(file)); | ||
| .any(|s| s.contains(file.as_str())); | ||
| if !has_it && rule_path.exists() { | ||
| arr.push(json!(path_str)); | ||
| added += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Substring/suffix filename matching can mismatch when driven by user-chosen coaching rule ids.
Both the prune retain (normalized.ends_with(f.as_str())) and the add-dedup check (s.contains(file.as_str())) match on partial filename text rather than exact basenames. expected_filenames now includes arbitrary user-supplied coaching ids (up to 10 alnum/hyphen chars), so two ids that happen to share a suffix (e.g. "ab" and "cab" → ab.md/cab.md) can cause a stale entry to survive pruning, or a genuinely-missing entry to be skipped as "already present." Comparing exact basenames (e.g. Path::file_name() equality) instead of ends_with/contains would remove this risk.
🤖 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/init.rs` around lines 494 - 521, Update the pruning and deduplication
logic around expected_filenames to compare exact path basenames rather than
filename substrings. In the retain closure, extract each entry’s basename and
match it exactly against the expected filename; in the has_it check, likewise
compare basenames exactly for both arr entries and sibling_instructions,
preserving the existing behavior for non-path values.
CI's fmt job was failing on unformatted coaching/dashboard/mcp_server changes, and clippy's --locked check was failing because Cargo.lock hadn't been regenerated after adding toml/thiserror/agentflare-store/ blake3 as dependencies.
…ult_large_err toml::de::Error is >128 bytes, so embedding it directly in LoaderError tripped clippy::result_large_err (denied via -D warnings) on every function returning Result<_, LoaderError>.
Root cause of the intermittent build (windows-latest) CI failures in state::tests::* and vent::capture::tests::*: with_temp_home/with_temp_cwd reused a single fixed directory name across every call. A mutex serialized the env-var mutation itself, but under cargo test's default parallel runner and heavy concurrent filesystem load elsewhere in the 811-test suite, a previous call's directory could still be non-empty (or its file handles not yet released) by the time the next call reused the same path, leaking persisted state (SQLite store contents, vent log entries) from one test into an unrelated one. Fixed by giving each call a uniquely-named tempfile::tempdir() instead of a shared fixed name, so no two calls can ever collide on the same directory regardless of timing. Also made both helpers panic-safe via Drop guards, so a failing assertion inside the wrapped closure can no longer leave AGENTFLARE_HOME_OVERRIDE (or the cwd) permanently altered for whatever the test binary runs next. Verified: 7 consecutive clean cargo test --workspace / -p agentflare runs (0 failures) after the fix, versus 100% reproducible failure before it. Added two regression tests exercising with_temp_home under real thread contention.
# Conflicts: # Cargo.lock
1 participant
Scope: crate-only —
crates/flare-git-core/Implements the git-shim slice of EPIC #331 (unified
~/.agentflare/config.toml): makes three hardcoded policy lists inclassify.rsuser-adjustable.Changes
config_loader.rs(new)~/.agentflare/config.toml+<repo>/.agentflare/config.toml)policy_config.rs(new)ResolvedGitShimPolicy; additive-only (union)classify.rs&[String]/&ResolvedGitShimPolicythroughclassify_pure,resolve_trust_root_touch; fail-safe fallback on malformed configlib.rsCargo.tomltoml = "0.8",thiserror = "2"depsDesign decisions
Verification
cargo test -p flare-git-core→ 125/125 pass (was 124 before, +1 e2e test)cargo fmtcleanCannot run workspace build here — C: disk full (see item #366 notes).
Summary by CodeRabbit
synctargets, plus a new sync command to fan out updates across hosts..agentflaredeletes).