checkpoint

⚒️ Generated with [Fabro](https://fabro.sh)
This commit is contained in:
Fabro 2026-03-16 01:36:42 -04:00
parent 8a200c92a1
commit 98eee7feaa
6 changed files with 363 additions and 50 deletions

File diff suppressed because one or more lines are too long

View file

@ -0,0 +1,82 @@
diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs
index d7afe28..36bf56e 100644
--- a/lib/crates/fabro-cli/src/main.rs
+++ b/lib/crates/fabro-cli/src/main.rs
@@ -814,12 +814,7 @@ async fn main_inner() -> (String, Result<()>) {
}
},
Command::SendAnalytics { path } => {
- let result = async {
- let json = std::fs::read(&path)?;
- let track: fabro_util::telemetry::event::Track = serde_json::from_slice(&json)?;
- fabro_util::telemetry::sender::send_to_segment(&track).await
- }
- .await;
+ let result = fabro_util::telemetry::sender::send_to_segment(&path).await;
let _ = std::fs::remove_file(&path);
result?;
}
diff --git a/lib/crates/fabro-util/src/telemetry/panic.rs b/lib/crates/fabro-util/src/telemetry/panic.rs
index d21ee6d..2cad1c3 100644
--- a/lib/crates/fabro-util/src/telemetry/panic.rs
+++ b/lib/crates/fabro-util/src/telemetry/panic.rs
@@ -52,7 +52,7 @@ pub fn build_panic_event(message: &str) -> Event<'static> {
);
// Set release to the package version.
- event.release = Some(env!("CARGO_PKG_VERSION").into());
+ event.release = Some(crate::version::FABRO_VERSION.into());
event
}
@@ -111,13 +111,7 @@ pub async fn send_panic_to_sentry(path: &Path) -> anyhow::Result<()> {
let json = std::fs::read(path)?;
let event: Event<'static> = serde_json::from_slice(&json)?;
- let guard = sentry::init((
- dsn,
- sentry::ClientOptions {
- release: Some(env!("CARGO_PKG_VERSION").into()),
- ..Default::default()
- },
- ));
+ let guard = sentry::init((dsn, sentry::ClientOptions::default()));
sentry::capture_event(event);
diff --git a/lib/crates/fabro-util/src/telemetry/sender.rs b/lib/crates/fabro-util/src/telemetry/sender.rs
index 27e636f..bf89ce7 100644
--- a/lib/crates/fabro-util/src/telemetry/sender.rs
+++ b/lib/crates/fabro-util/src/telemetry/sender.rs
@@ -1,3 +1,5 @@
+use std::path::Path;
+
use base64::engine::general_purpose::STANDARD;
use base64::Engine;
@@ -30,17 +32,22 @@ fn spawn_sender(track: Track) {
super::spawn::spawn_fabro_subcommand("__send_analytics", &filename, &json);
}
-/// Sends a track event to Segment. Called by the `__send_analytics` subcommand.
-pub async fn send_to_segment(track: &Track) -> anyhow::Result<()> {
+/// Reads a serialized track event from `path` and sends it to Segment.
+/// Called by the `__send_analytics` subcommand.
+/// No-ops if `SEGMENT_WRITE_KEY` was not set at compile time.
+pub async fn send_to_segment(path: &Path) -> anyhow::Result<()> {
let write_key = SEGMENT_WRITE_KEY
.ok_or_else(|| anyhow::anyhow!("SEGMENT_WRITE_KEY not set at compile time"))?;
+ let json = std::fs::read(path)?;
+ let track: Track = serde_json::from_slice(&json)?;
+
let auth = STANDARD.encode(format!("{write_key}:"));
let resp = reqwest::Client::new()
.post(SEGMENT_API_URL)
.header("Authorization", format!("Basic {auth}"))
- .json(track)
+ .json(&track)
.send()
.await?;

View file

@ -0,0 +1,176 @@
Goal: # Add Sentry panic reporting to fabro CLI
## Context
Fabro has no panic reporting. When the CLI panics, we lose visibility. We want Sentry-based panic reporting modeled on qlty's approach: serialize a Sentry event to a temp file, double-fork a detached subprocess to upload it.
The existing `__send_analytics` sender uses a simple `Command::spawn()` which is unreliable — the child can get killed when the parent exits or the terminal session ends. We'll fix both senders to use the double-fork pattern (fork → setsid → close_fd → fork → exec) from qlty, which fully detaches the subprocess.
## Files to modify
### 1. `Cargo.toml` (workspace root) — add deps
```toml
sentry = { version = "0.35", default-features = false, features = ["backtrace", "contexts", "ureq", "rustls"] }
fork = "0.2"
exec = "0.3"
```
### 2. `lib/crates/fabro-util/Cargo.toml` — add deps
```toml
sentry.workspace = true
fork.workspace = true
exec.workspace = true
```
### 3. `lib/crates/fabro-util/src/telemetry/mod.rs`
- Add `pub mod panic;`
- Add `pub mod spawn;`
### 4. `lib/crates/fabro-util/src/telemetry/spawn.rs` — NEW FILE
Extract a shared `spawn_detached(args: &[&str], env: &[(&str, &str)])` function used by both analytics sender and panic sender. Implements:
- **Unix**: double-fork via `fork` crate (fork → setsid → close_fd → fork → exec)
- **Windows**: `Command::new().creation_flags(DETACHED_PROCESS).spawn()`
### 5. `lib/crates/fabro-util/src/telemetry/sender.rs` — refactor to use `spawn_detached`
Replace the `Command::new().spawn()` call with `spawn::spawn_detached()`.
### 6. `lib/crates/fabro-util/src/telemetry/panic.rs` — NEW FILE
Core logic:
- `install_panic_hook()` — chains onto default hook (like qlty). Calls `report_panic()` then delegates to default.
- `report_panic(info: &PanicHookInfo)` — checks telemetry level (Off → return), filters "Broken pipe" panics, builds `sentry::protocol::Event` with:
- Exception with `mechanism.ty = "panic"`, `handled = false`
- Panic message as `value`
- `sentry_backtrace::current_stacktrace()`
- OS context
- Level = Fatal
- `spawn_panic_sender(event)` — serialize event to `~/.fabro/tmp/fabro-panic-{id}.json`, double-fork `fabro __send_panic <path>` with `FABRO_TELEMETRY=off`.
- `pub async fn send_panic_to_sentry(path: &Path)` — read JSON, init sentry client with `SENTRY_DSN` (compile-time `option_env!`), `sentry::capture_event()`, delete temp file.
### 7. `lib/crates/fabro-cli/src/main.rs` — two changes
**a) Install panic hook early in `main()`** (before `main_inner()`):
```rust
fabro_util::telemetry::panic::install_panic_hook();
```
**b) Add `__send_panic` hidden subcommand** (same pattern as `__send_analytics`).
## Key design decisions
- **Double-fork for all background senders** — ensures subprocess survives parent exit and terminal close. Fixes existing `__send_analytics` reliability too.
- **Shared `spawn_detached`** — avoids duplicating the fork logic between analytics and panic senders.
- **Telemetry level respected** — `Off` skips panic reporting. `Errors` and `All` both report.
- **Recursion prevention** — subprocess sets `FABRO_TELEMETRY=off`.
- **Compile-time DSN** — `option_env!("SENTRY_DSN")`, no-ops when unset (dev builds).
- **Filter broken pipe** — CLI tools get SIGPIPE from `| head` etc., not a real bug.
## Implementation order (red/green TDD)
Work bottom-up through the dependency chain. For each step, write the test first (red), then write the minimal code to make it pass (green).
### Step 1: `spawn_detached` (fabro-util)
- **Red**: Test that `spawn_detached` constructs the right args (unit-testable parts: temp file writing, arg assembly). On Unix, test the double-fork integration by spawning a real process that writes a marker file.
- **Green**: Implement `spawn.rs` with fork/setsid/close_fd/fork/exec on Unix, DETACHED_PROCESS on Windows.
### Step 2: Refactor `sender.rs` to use `spawn_detached`
- **Red**: Existing `send_noops_without_write_key` test still passes. Add test verifying temp file is written with correct JSON.
- **Green**: Replace `Command::spawn()` with `spawn_detached()`.
### Step 3: `panic.rs` — event building
- **Red**: Test `build_panic_event()` produces correct Sentry event structure (exception type, mechanism, level, stacktrace present).
- **Green**: Implement event builder.
### Step 4: `panic.rs` — hook + spawn
- **Red**: Test that `report_panic` filters broken pipe. Test that it no-ops when telemetry is Off.
- **Green**: Implement `install_panic_hook()`, `report_panic()`, `spawn_panic_sender()`.
### Step 5: `panic.rs` — Sentry sender
- **Red**: Test `send_panic_to_sentry` no-ops without DSN (like analytics no-ops without write key).
- **Green**: Implement the `__send_panic` worker function.
### Step 6: Wire up in `main.rs`
- Add `__send_panic` subcommand + install panic hook.
- Verify: `cargo build --workspace && cargo test --workspace && cargo clippy --workspace -- -D warnings`
## Completed stages
- **toolchain**: success
- Script: `command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1`
- Stdout:
```
cargo 1.94.0 (85eff7c80 2026-01-15)
```
- Stderr: (empty)
- **preflight_compile**: success
- Script: `cargo check -q --workspace 2>&1`
- Stdout: (empty)
- Stderr: (empty)
- **preflight_lint**: success
- Script: `cargo clippy -q --workspace -- -D warnings 2>&1`
- Stdout: (empty)
- Stderr: (empty)
- **implement**: success
- Model: claude-opus-4-6, 89.1k tokens in / 13.5k out
- Files: /home/daytona/workspace/lib/crates/fabro-cli/src/main.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/mod.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/panic.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/sender.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/spawn.rs
- **simplify_opus**: success
- Model: claude-opus-4-6, 60.2k tokens in / 17.7k out
- Files: /home/daytona/workspace/Cargo.toml, /home/daytona/workspace/lib/crates/fabro-util/Cargo.toml, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/panic.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/sender.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/spawn.rs
- **simplify_gemini**: success
- Model: claude-opus-4-6, 54.4k tokens in / 12.4k out
- Files: /home/daytona/workspace/lib/crates/fabro-cli/src/main.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/panic.rs, /home/daytona/workspace/lib/crates/fabro-util/src/telemetry/sender.rs
# Simplify: Code Review and Cleanup
Review all changed files for reuse, quality, and efficiency. Fix any issues found.
## Phase 1: Identify Changes
Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.
## Phase 2: Launch Three Review Agents in Parallel
Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.
### Agent 1: Code Reuse Review
For each change:
1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
Note: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.
### Agent 2: Code Quality Review
Review the same changes for hacky patterns:
1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
Note: This is a greenfield app, so be aggressive in optimizing quality.
### Agent 3: Efficiency Review
Review the same changes for efficiency:
1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
2. Missed concurrency: independent operations run sequentially when they could run in parallel
3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
5. Memory: unbounded data structures, missing cleanup, event listener leaks
6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one
## Phase 3: Fix Issues
Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.
When done, briefly summarize what was fixed (or confirm the code was already clean).

View file

@ -0,0 +1,5 @@
{
"mode": "agent",
"provider": "anthropic",
"model": "claude-opus-4-6"
}

View file

@ -0,0 +1,16 @@
Good. Here's a summary of the review and fixes:
## Review Summary
### Issues Fixed
1. **Missing `SENTRY_DSN` early check in `report_panic`** (`panic.rs` line 78-81): Added `if SENTRY_DSN.is_none() { return; }` at the top of `report_panic()`. This follows the same pattern as `sender.rs` which checks `SEGMENT_WRITE_KEY.is_none()` early in `send()`. Without this, a panic in a release build compiled without `SENTRY_DSN` would unnecessarily: capture a stacktrace, build a Sentry event, serialize it to JSON, write a temp file, and fork a subprocess — only for the subprocess to immediately error with "SENTRY_DSN not set".
2. **Misleading doc comment on `send_panic_to_sentry`** (`panic.rs` line 110): Changed "Reads the JSON event from `path`, sends it to Sentry, then deletes the file." to "Reads the JSON event from `path` and sends it to Sentry." — the function doesn't delete the file; `main.rs` handles cleanup.
### Reviewed and Clean (no issues)
- **Code reuse**: `spawn_fabro_subcommand` correctly centralizes the temp-file-write + detached-subprocess pattern shared by both analytics and panic senders. The `dirs::home_dir().join(".fabro")` pattern is used ad-hoc across the codebase (no existing helper to reuse).
- **Copy-paste**: The `SendAnalytics` and `SendPanic` handlers in `main.rs` are 3 lines each with different function calls — not worth abstracting further.
- **Spawn logic**: The double-fork pattern is well-implemented and correctly isolated in `spawn.rs`. The existing `detach_run` in `main.rs` is a different use case (redirects to log files) so it doesn't make sense to share.
- **Efficiency**: `install_panic_hook()` is a lightweight `take_hook + set_hook` — no startup overhead. The stacktrace capture in `build_panic_event` is expensive but only runs on actual panics. `sentry::init` in the subprocess is fine since it's a separate process.

View file

@ -0,0 +1,6 @@
{
"status": "success",
"notes": "Stage completed: simplify_gpt",
"failure_reason": null,
"timestamp": "2026-03-16T05:36:42.161154+00:00"
}