Skip to content

feat: Remove Agent Trace persistence and reset hooks to no-op baseline#23

Open
davidabram wants to merge 6 commits intomainfrom
remove-agent-trace
Open

feat: Remove Agent Trace persistence and reset hooks to no-op baseline#23
davidabram wants to merge 6 commits intomainfrom
remove-agent-trace

Conversation

@davidabram
Copy link
Copy Markdown
Member

@davidabram davidabram commented Apr 8, 2026

Remove the Agent Trace implementation from the CLI runtime to establish a clean v0.3.0 redesign baseline

Summary by CodeRabbit

  • Chores

    • Removed the "trace" and "sync" CLI commands and their entries from top-level help.
    • Dropped local DB persistence for agent trace data; trace persistence and related health/migration checks removed.
    • Diagnostics no longer report or auto-fix an Agent Trace local DB; default persisted-paths simplified and renamed.
  • New Features

    • Added a runtime/config flag and schema support to enable attribution-only git hooks (disabled by default).

Remove the Agent Trace implementation from the CLI runtime to establish
a clean v0.3.0 redesign baseline

Co-authored-by: SCE <sce@crocoder.dev>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e584bad8-29e3-4725-9032-d89abc95e053

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee2e80 and bcee76c.

⛔ Files ignored due to path filters (17)
  • cli/Cargo.lock is excluded by !**/*.lock
  • context/architecture.md is excluded by !context/**/*.md
  • context/cli/cli-command-surface.md is excluded by !context/**/*.md
  • context/context-map.md is excluded by !context/**/*.md
  • context/glossary.md is excluded by !context/**/*.md
  • context/overview.md is excluded by !context/**/*.md
  • context/patterns.md is excluded by !context/**/*.md
  • context/plans/agent-trace-removal-and-hook-noop-reset.md is excluded by !context/**/*.md
  • context/sce/agent-trace-implementation-contract.md is excluded by !context/**/*.md
  • context/sce/agent-trace-local-hooks-mvp-contract-gap-matrix.md is excluded by !context/**/*.md
  • context/sce/agent-trace-payload-builder-validation.md is excluded by !context/**/*.md
  • context/sce/agent-trace-post-commit-dual-write.md is excluded by !context/**/*.md
  • context/sce/agent-trace-post-rewrite-local-remap-ingestion.md is excluded by !context/**/*.md
  • context/sce/agent-trace-pre-commit-staged-checkpoint.md is excluded by !context/**/*.md
  • context/sce/agent-trace-retry-queue-observability.md is excluded by !context/**/*.md
  • context/sce/agent-trace-rewrite-trace-transformation.md is excluded by !context/**/*.md
  • context/sce/agent-trace-schema-adapter.md is excluded by !context/**/*.md
📒 Files selected for processing (5)
  • cli/Cargo.toml
  • cli/src/services/default_paths.rs
  • cli/src/services/doctor.rs
  • cli/src/services/local_db.rs
  • cli/src/services/mod.rs
💤 Files with no reviewable changes (3)
  • cli/Cargo.toml
  • cli/src/services/mod.rs
  • cli/src/services/local_db.rs

📝 Walkthrough

Walkthrough

Removed the trace and sync CLI subcommands and their modules; replaced local-DB persistence/migration logic with no-op implementations; added an attribution_hooks.enabled runtime config flag and schema; updated command-surface/help text and doctor/health checks to drop agent-trace DB specifics.

Changes

Cohort / File(s) Summary
CLI parsing & dispatch
cli/src/app.rs, cli/src/cli_schema.rs
Deleted trace and sync CLI variants and TraceSubcommand; removed parsing-to-internal conversions and dispatch branches for those commands.
Command surface / help
cli/src/command_surface.rs
Removed Placeholder variant and trace/sync entries from COMMANDS; updated hooks purpose/help text.
Removed service modules & re-exports
cli/src/services/trace.rs, cli/src/services/sync.rs, cli/src/services/mod.rs
Deleted trace and sync modules and removed their pub mod re-exports.
Agent-trace payload code removed
cli/src/services/agent_trace.rs
Entire agent-trace payload builder/adaptor module and related types/constants deleted.
Hooks persistence & runtime flows
cli/src/services/hooks.rs
Replaced LocalDb stores/ingestion with NoOp implementations; removed DB write, idempotency, and retry logic; simplified runtime path resolution and readiness calls.
Local DB API, migrations & defaults
cli/src/services/local_db.rs, cli/src/services/default_paths.rs
Removed local_db module (schema/migrations/smoke-checks) and agent-trace persisted artifact; added resolve_state_data_root(); eliminated agent-trace-specific path helpers.
Doctor / health reporting
cli/src/services/doctor.rs
Removed agent-trace local DB health checks, problems, report fields, and auto-fix paths; updated dependencies to use generic state-root resolver.
Runtime hook configuration & schema
cli/src/services/config.rs, config/schema/sce-config.schema.json
Added attribution_hooks.enabled to RuntimeConfig and resolver APIs; extended JSON Schema with policies.attribution_hooks.enabled and tightened allowlisting.
Build / deps
cli/Cargo.toml
Removed turso dependency.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Preparing 0.2.0 #18: Modifies CLI command surface and related command/help handling; closely related to removal/hiding of trace and sync commands.

Suggested reviewers

  • ivke995
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main objective: removing Agent Trace persistence and resetting hooks to a no-op baseline, which aligns with all major file changes across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch remove-agent-trace

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/src/services/hooks.rs (1)

766-780: ⚠️ Potential issue | 🟠 Major

Don't keep local DB warmup on the hook critical path.

let _ = ensure_agent_trace_local_db_ready_blocking()?; is still a hard failure. With NoOpTraceRecordStore in both post-commit and post-rewrite, a broken/unwritable state directory now causes both hooks to skip git-notes emission and retry-queue replay even though the remaining persistence lives under .git. It also recreates the Agent Trace DB as a side effect of "resolving" runtime paths.

Suggested fix
 fn resolve_post_commit_runtime_paths(repository_root: &Path) -> Result<PostCommitRuntimePaths> {
-    let _ = ensure_agent_trace_local_db_ready_blocking()?;
     let retry_queue_path = resolve_git_path(
         repository_root,
         default_paths::git_relative_path::TRACE_RETRY_QUEUE,
     )?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/services/hooks.rs` around lines 766 - 780, The call to
ensure_agent_trace_local_db_ready_blocking() inside
resolve_post_commit_runtime_paths makes the hook fail hard and recreates the
Agent Trace DB; remove this blocking warmup from
resolve_post_commit_runtime_paths (or make it non-fatal) so the function only
resolves paths (retry_queue_path and emission_ledger_path) and returns
PostCommitRuntimePaths; if you still want a best-effort warmup, invoke
ensure_agent_trace_local_db_ready_blocking() outside the hook critical path (or
catch and log its error and continue) and ensure NoOpTraceRecordStore behavior
remains unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/src/services/hooks.rs`:
- Around line 1353-1355: The NoOpRewriteRemapIngestion currently returns
Ok(true) unconditionally which prevents skipped_pairs from ever incrementing;
update the ingestion logic in the NoOpRewriteRemapIngestion handler (the code
that receives remap pairs and mutates accepted_requests) to check for existing
remap pairs in accepted_requests before accepting: if the pair already exists,
do not push it and return Ok(false) to indicate a duplicate was skipped; if it’s
new, push into accepted_requests and return Ok(true). Apply the same fix to the
other duplicate block referenced (the similar handler around the 1538-1546
region) so duplicate-skipping semantics and post-rewrite counts remain accurate.

---

Outside diff comments:
In `@cli/src/services/hooks.rs`:
- Around line 766-780: The call to ensure_agent_trace_local_db_ready_blocking()
inside resolve_post_commit_runtime_paths makes the hook fail hard and recreates
the Agent Trace DB; remove this blocking warmup from
resolve_post_commit_runtime_paths (or make it non-fatal) so the function only
resolves paths (retry_queue_path and emission_ledger_path) and returns
PostCommitRuntimePaths; if you still want a best-effort warmup, invoke
ensure_agent_trace_local_db_ready_blocking() outside the hook critical path (or
catch and log its error and continue) and ensure NoOpTraceRecordStore behavior
remains unaffected.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: db61c1e3-2ca3-4b33-a27e-4877c1beb8b9

📥 Commits

Reviewing files that changed from the base of the PR and between 90717d6 and 4238faf.

⛔ Files ignored due to path filters (11)
  • context/architecture.md is excluded by !context/**/*.md
  • context/context-map.md is excluded by !context/**/*.md
  • context/glossary.md is excluded by !context/**/*.md
  • context/overview.md is excluded by !context/**/*.md
  • context/patterns.md is excluded by !context/**/*.md
  • context/plans/agent-trace-removal-and-hook-noop-reset.md is excluded by !context/**/*.md
  • context/sce/agent-trace-core-schema-migrations.md is excluded by !context/**/*.md
  • context/sce/agent-trace-local-hooks-mvp-contract-gap-matrix.md is excluded by !context/**/*.md
  • context/sce/agent-trace-post-commit-dual-write.md is excluded by !context/**/*.md
  • context/sce/agent-trace-reconciliation-schema-ingestion.md is excluded by !context/**/*.md
  • context/sce/agent-trace-rewrite-trace-transformation.md is excluded by !context/**/*.md
📒 Files selected for processing (7)
  • cli/src/app.rs
  • cli/src/cli_schema.rs
  • cli/src/command_surface.rs
  • cli/src/services/hooks.rs
  • cli/src/services/local_db.rs
  • cli/src/services/mod.rs
  • cli/src/services/sync.rs
💤 Files with no reviewable changes (5)
  • cli/src/services/mod.rs
  • cli/src/cli_schema.rs
  • cli/src/app.rs
  • cli/src/services/sync.rs
  • cli/src/command_surface.rs

Remove the full agent trace pipeline (pre-commit checkpointing, post-commit
trace persistence, retry queues, emission ledger) from git hooks. The hooks
now operate in a simplified attribution-only mode that only manages the
SCE co-author trailer in commit messages.

Add `policies.attribution_hooks.enabled` config option and
`SCE_ATTRIBUTION_HOOKS_ENABLED` environment variable to control whether
attribution hooks are active. Defaults to false.

Co-authored-by: SCE <sce@crocoder.dev>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/src/services/config.rs (1)

1729-1731: 🧹 Nitpick | 🔵 Trivial

Consider including attribution_hooks_enabled in config show output.

The new attribution_hooks_enabled setting is not displayed in the config show output (neither text nor JSON format). While this may be intentional since it's a hook-specific runtime setting, including it would help users verify their configuration during troubleshooting.

♻️ Proposed addition to JSON output
 "policies": {
-    "bash": format_bash_policies_json(&runtime.bash_policies),
+    "bash": format_bash_policies_json(&runtime.bash_policies),
+    "attribution_hooks": format_resolved_value_json(
+        runtime.attribution_hooks_enabled.value,
+        runtime.attribution_hooks_enabled.source,
+    ),
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/services/config.rs` around lines 1729 - 1731, The config show output
currently emits "policies" JSON but omits the new runtime flag; update the
formatter that builds the config show output to include
runtime.attribution_hooks_enabled in both the JSON and text representations.
Locate the block that constructs the JSON map (where "policies": { "bash":
format_bash_policies_json(&runtime.bash_policies) } is added) and add an
"attribution_hooks_enabled": runtime.attribution_hooks_enabled entry, and mirror
that in the textual output generator (the same function that prints runtime
fields for config show) so users see the flag in both formats.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/src/services/config.rs`:
- Around line 292-295: The struct ResolvedHookRuntimeConfig currently derives
Clone, Debug, Eq, PartialEq but should also derive Copy because it contains only
a bool; update the derive attribute on ResolvedHookRuntimeConfig to include Copy
and follow the guideline order (Clone, Copy, Debug, Eq, PartialEq) so instances
can be copied trivially instead of moved.

---

Outside diff comments:
In `@cli/src/services/config.rs`:
- Around line 1729-1731: The config show output currently emits "policies" JSON
but omits the new runtime flag; update the formatter that builds the config show
output to include runtime.attribution_hooks_enabled in both the JSON and text
representations. Locate the block that constructs the JSON map (where
"policies": { "bash": format_bash_policies_json(&runtime.bash_policies) } is
added) and add an "attribution_hooks_enabled": runtime.attribution_hooks_enabled
entry, and mirror that in the textual output generator (the same function that
prints runtime fields for config show) so users see the flag in both formats.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d59b8108-2628-4309-be18-3563dbc19a35

📥 Commits

Reviewing files that changed from the base of the PR and between 4238faf and 57f94e2.

⛔ Files ignored due to path filters (14)
  • config/pkl/base/sce-config-schema.pkl is excluded by !**/*.pkl
  • context/architecture.md is excluded by !context/**/*.md
  • context/context-map.md is excluded by !context/**/*.md
  • context/glossary.md is excluded by !context/**/*.md
  • context/overview.md is excluded by !context/**/*.md
  • context/patterns.md is excluded by !context/**/*.md
  • context/plans/agent-trace-removal-and-hook-noop-reset.md is excluded by !context/**/*.md
  • context/sce/agent-trace-commit-msg-coauthor-policy.md is excluded by !context/**/*.md
  • context/sce/agent-trace-hooks-command-routing.md is excluded by !context/**/*.md
  • context/sce/agent-trace-post-commit-dual-write.md is excluded by !context/**/*.md
  • context/sce/agent-trace-post-rewrite-local-remap-ingestion.md is excluded by !context/**/*.md
  • context/sce/agent-trace-pre-commit-staged-checkpoint.md is excluded by !context/**/*.md
  • context/sce/agent-trace-retry-queue-observability.md is excluded by !context/**/*.md
  • context/sce/agent-trace-rewrite-trace-transformation.md is excluded by !context/**/*.md
📒 Files selected for processing (3)
  • cli/src/services/config.rs
  • cli/src/services/hooks.rs
  • config/schema/sce-config.schema.json

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/src/services/local_db.rs (1)

18-37: 🧹 Nitpick | 🔵 Trivial

Add path/step context to local DB health failures.

Lines 18-37 bubble raw DB errors, so sce doctor loses whether runtime creation, open, connect, init, or PRAGMA schema_version failed for a specific file. Wrapping each fallible step with Context/with_context would make failures much more actionable.

♻️ Suggested change
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};
 use turso::Builder;
@@
 pub(crate) fn check_local_db_health_blocking(path: &Path) -> Result<()> {
     let runtime = tokio::runtime::Builder::new_current_thread()
         .enable_time()
-        .build()?;
+        .build()
+        .context("Failed to create runtime for local DB health check.")?;
     runtime.block_on(check_local_db_health(path))
 }
 
 async fn check_local_db_health(path: &Path) -> Result<()> {
-    let conn = connect_local(LocalDatabaseTarget::Path(path)).await?;
-    let mut rows = conn.query("PRAGMA schema_version", ()).await?;
+    let conn = connect_local(LocalDatabaseTarget::Path(path))
+        .await
+        .with_context(|| format!("Failed to open local DB '{}'.", path.display()))?;
+    let mut rows = conn
+        .query("PRAGMA schema_version", ())
+        .await
+        .with_context(|| format!("Failed to query local DB '{}'.", path.display()))?;
     let _ = rows.next().await?;
     Ok(())
 }
 
 async fn connect_local(target: LocalDatabaseTarget<'_>) -> Result<turso::Connection> {
     let location = target_location(target)?;
-    let db = Builder::new_local(location).build().await?;
-    let conn = db.connect()?;
-    conn.execute("PRAGMA foreign_keys = ON", ()).await?;
+    let db = Builder::new_local(location)
+        .build()
+        .await
+        .with_context(|| format!("Failed to build local DB '{}'.", location))?;
+    let conn = db
+        .connect()
+        .with_context(|| format!("Failed to connect local DB '{}'.", location))?;
+    conn.execute("PRAGMA foreign_keys = ON", ())
+        .await
+        .with_context(|| format!("Failed to initialize local DB '{}'.", location))?;
     Ok(conn)
 }

As per coding guidelines, "Add context to I/O and process failures in Rust with Context / with_context."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/services/local_db.rs` around lines 18 - 37, Wrap each fallible step
with Context/with_context to add the file path and the specific operation so
errors show where they failed: add context on
tokio::runtime::Builder::new_current_thread().build() in
check_local_db_health_blocking, on
connect_local(LocalDatabaseTarget::Path(path)) and inside connect_local around
Builder::new_local(location).build(), db.connect(), conn.execute("PRAGMA
foreign_keys = ON", ...), conn.query("PRAGMA schema_version", ...), and
rows.next().await; include the path and a short step message (e.g. "creating
runtime", "opening DB at {path}", "connecting to DB", "enabling foreign_keys",
"querying schema_version", "reading query row") using Context/with_context so
failures report both the path and which step failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/src/services/default_paths.rs`:
- Line 81: The change orphaned the legacy Agent Trace DB by moving the canonical
DB to a new path and removing the legacy artifact location; update
default_paths.rs to detect a legacy trace DB on startup (when handling the
LocalDb/default path resolution) and perform a one-time migration: if the legacy
file exists and the new canonical file is absent, move or copy the legacy DB to
the new canonical location (or import its contents), then remove the legacy
file; alternatively, until a migration is implemented, restore reporting of the
legacy file from the code paths that previously surfaced it so existing installs
remain visible until cleanup is done.

In `@cli/src/services/doctor.rs`:
- Around line 1239-1244: The change renamed the CLI label/JSON field for the
local DB which breaks existing JSON consumers; update the doctor output code
(references: local_db_status and report.local_db) to emit both the old and new
JSON keys during a deprecation window (e.g., include both "localDb" and
"local_db" or the previous key and the new "local_db") or gate the new shape
behind a versioned --format flag, and ensure the human-facing label change
("Local DB") remains unchanged; adjust the serialization logic that builds the
JSON payload (where report.local_db is used) to populate both keys or
conditionally choose the new key when a versioned format is requested.

---

Outside diff comments:
In `@cli/src/services/local_db.rs`:
- Around line 18-37: Wrap each fallible step with Context/with_context to add
the file path and the specific operation so errors show where they failed: add
context on tokio::runtime::Builder::new_current_thread().build() in
check_local_db_health_blocking, on
connect_local(LocalDatabaseTarget::Path(path)) and inside connect_local around
Builder::new_local(location).build(), db.connect(), conn.execute("PRAGMA
foreign_keys = ON", ...), conn.query("PRAGMA schema_version", ...), and
rows.next().await; include the path and a short step message (e.g. "creating
runtime", "opening DB at {path}", "connecting to DB", "enabling foreign_keys",
"querying schema_version", "reading query row") using Context/with_context so
failures report both the path and which step failed.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 630b0c03-1902-4289-ac79-867bfe833141

📥 Commits

Reviewing files that changed from the base of the PR and between 57f94e2 and 60605bd.

⛔ Files ignored due to path filters (10)
  • context/architecture.md is excluded by !context/**/*.md
  • context/cli/cli-command-surface.md is excluded by !context/**/*.md
  • context/cli/default-path-catalog.md is excluded by !context/**/*.md
  • context/context-map.md is excluded by !context/**/*.md
  • context/glossary.md is excluded by !context/**/*.md
  • context/overview.md is excluded by !context/**/*.md
  • context/patterns.md is excluded by !context/**/*.md
  • context/plans/agent-trace-removal-and-hook-noop-reset.md is excluded by !context/**/*.md
  • context/sce/agent-trace-core-schema-migrations.md is excluded by !context/**/*.md
  • context/sce/agent-trace-hook-doctor.md is excluded by !context/**/*.md
📒 Files selected for processing (8)
  • cli/src/app.rs
  • cli/src/cli_schema.rs
  • cli/src/command_surface.rs
  • cli/src/services/default_paths.rs
  • cli/src/services/doctor.rs
  • cli/src/services/local_db.rs
  • cli/src/services/mod.rs
  • cli/src/services/trace.rs
💤 Files with no reviewable changes (3)
  • cli/src/services/mod.rs
  • cli/src/app.rs
  • cli/src/services/trace.rs

Delete the agent_trace module which contained trace record structures,
VCS metadata constants, and payload adapter functions that are no
longer referenced by the codebase.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
cli/src/services/local_db.rs (2)

17-35: 🛠️ Refactor suggestion | 🟠 Major

Add context before bubbling local DB failures up to doctor.

cli/src/services/doctor.rs:628-642 renders {error} directly in the user-facing summary, but all of the runtime/DB calls here use bare ?. If runtime construction, DB open, PRAGMA foreign_keys, or PRAGMA schema_version fails, the report will not say which step or path failed.

♻️ Proposed change
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};

 pub(crate) fn check_local_db_health_blocking(path: &Path) -> Result<()> {
     let runtime = tokio::runtime::Builder::new_current_thread()
         .enable_time()
-        .build()?;
-    runtime.block_on(check_local_db_health(path))
+        .build()
+        .context("failed to construct local DB health-check runtime")?;
+    runtime
+        .block_on(check_local_db_health(path))
+        .with_context(|| format!("failed to check local DB '{}'", path.display()))
 }

 async fn check_local_db_health(path: &Path) -> Result<()> {
-    let conn = connect_local(LocalDatabaseTarget::Path(path)).await?;
-    let mut rows = conn.query("PRAGMA schema_version", ()).await?;
-    let _ = rows.next().await?;
+    let conn = connect_local(LocalDatabaseTarget::Path(path))
+        .await
+        .with_context(|| format!("failed to open local DB '{}'", path.display()))?;
+    let mut rows = conn
+        .query("PRAGMA schema_version", ())
+        .await
+        .with_context(|| format!("failed to query local DB '{}'", path.display()))?;
+    let _ = rows
+        .next()
+        .await
+        .with_context(|| format!("failed to read PRAGMA schema_version from '{}'", path.display()))?;
     Ok(())
 }

 async fn connect_local(target: LocalDatabaseTarget<'_>) -> Result<turso::Connection> {
     let location = target_location(target)?;
-    let db = Builder::new_local(location).build().await?;
-    let conn = db.connect()?;
-    conn.execute("PRAGMA foreign_keys = ON", ()).await?;
+    let db = Builder::new_local(location)
+        .build()
+        .await
+        .with_context(|| format!("failed to build local DB '{}'", location))?;
+    let conn = db
+        .connect()
+        .with_context(|| format!("failed to connect local DB '{}'", location))?;
+    conn.execute("PRAGMA foreign_keys = ON", ())
+        .await
+        .with_context(|| format!("failed to enable foreign keys for '{}'", location))?;
     Ok(conn)
 }
As per coding guidelines, "Add context to I/O and process failures in Rust with `Context` / `with_context`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/services/local_db.rs` around lines 17 - 35, The health-check
functions leak raw errors; wrap each I/O/DB operation with contextual messages
using anyhow::Context (or .with_context) so the doctor summary can show which
step/path failed: add context to tokio runtime construction in
check_local_db_health_blocking (include the Path), to connect_local when calling
target_location and Builder::new_local/build and db.connect (include
target/location), and to the PRAGMA calls in check_local_db_health (specify
which PRAGMA and the path) so every `?` is replaced with a contextualized error
explaining the failing step and the database path.

17-29: 🛠️ Refactor suggestion | 🟠 Major

Add focused unit tests for the reduced health-check contract.

This file now defines the baseline local DB health behavior, but there is no in-file #[cfg(test)] coverage for the success and failure paths. Two tempdir-based tests would likely be enough here: one that accepts a valid SQLite file and one that rejects a non-database file.

As per coding guidelines, "Add unit tests close to the code they exercise" and "Test names are descriptive, behavior-oriented, and usually sentence-like with underscores."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/services/local_db.rs` around lines 17 - 29, Add an in-file
#[cfg(test)] module with two unit tests that exercise the reduced health-check
contract via the synchronous wrapper check_local_db_health_blocking: one test
(e.g., accepts_valid_sqlite_file) should create a temporary directory and a
valid SQLite file (open a connection or create a DB there) then call
check_local_db_health_blocking(path) and assert it returns Ok(()); the other
test (e.g., rejects_non_database_file) should create a temp file containing
non-SQLite data and assert check_local_db_health_blocking(path) returns an Err;
place tests next to the functions check_local_db_health_blocking and
check_local_db_health and give the tests descriptive, behavior-oriented names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/src/services/local_db.rs`:
- Around line 17-21: The current blocking wrapper for check_local_db_health
constructs a Tokio runtime with only timers enabled; update the runtime builder
in check_local_db_health_blocking so it enables Tokio I/O as well by using the
enable_all() (or otherwise enabling both time and io) on the runtime builder
before build(), ensuring the ad-hoc runtime supports async I/O used by
check_local_db_health.

---

Outside diff comments:
In `@cli/src/services/local_db.rs`:
- Around line 17-35: The health-check functions leak raw errors; wrap each
I/O/DB operation with contextual messages using anyhow::Context (or
.with_context) so the doctor summary can show which step/path failed: add
context to tokio runtime construction in check_local_db_health_blocking (include
the Path), to connect_local when calling target_location and
Builder::new_local/build and db.connect (include target/location), and to the
PRAGMA calls in check_local_db_health (specify which PRAGMA and the path) so
every `?` is replaced with a contextualized error explaining the failing step
and the database path.
- Around line 17-29: Add an in-file #[cfg(test)] module with two unit tests that
exercise the reduced health-check contract via the synchronous wrapper
check_local_db_health_blocking: one test (e.g., accepts_valid_sqlite_file)
should create a temporary directory and a valid SQLite file (open a connection
or create a DB there) then call check_local_db_health_blocking(path) and assert
it returns Ok(()); the other test (e.g., rejects_non_database_file) should
create a temp file containing non-SQLite data and assert
check_local_db_health_blocking(path) returns an Err; place tests next to the
functions check_local_db_health_blocking and check_local_db_health and give the
tests descriptive, behavior-oriented names.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5190da6b-57a8-41d2-94ff-e30c7c5e2e55

📥 Commits

Reviewing files that changed from the base of the PR and between 60605bd and 7ee2e80.

📒 Files selected for processing (3)
  • cli/src/services/agent_trace.rs
  • cli/src/services/local_db.rs
  • cli/src/services/mod.rs
💤 Files with no reviewable changes (2)
  • cli/src/services/mod.rs
  • cli/src/services/agent_trace.rs

Clarify that Agent Trace adapter, payload builder, persistence,
retry, and rewrite flows are no longer active runtime behavior.

Document the current attribution-only hook baseline
and empty-file local DB baseline, and mark T04 complete in the removal plan.
Remove the turso-based local database implementation and all associated
Agent Trace persistence behavior from the CLI. This eliminates the
local_db service, schema bootstrap, trace emission, and related doctor
checks while keeping hook entrypoints present for optional attribution
behavior (now gated and disabled by default).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant