diff --git a/.continue/checks/error-handling-patterns.md b/.continue/checks/error-handling-patterns.md new file mode 100644 index 0000000..5538306 --- /dev/null +++ b/.continue/checks/error-handling-patterns.md @@ -0,0 +1,113 @@ +--- +name: Error Handling Patterns +description: Ensure consistent error handling with proper context propagation, no silent failures, and correct use of anyhow vs thiserror. +--- + +# Error Handling Patterns + +## Context + +Threader uses two error handling crates: `thiserror` for domain-specific error types (e.g., `AuthError` in `src/auth/mod.rs`) and `anyhow` for general error propagation. As a background daemon, silent failures are especially dangerous — users won't see errors unless they check logs. The project enforces `#[warn(clippy::unwrap_used)]` to prevent panics, but this doesn't catch swallowed errors or missing context. + +## What to Check + +### 1. Silent Error Swallowing + +Errors must not be silently ignored. Every error should be logged, propagated, or explicitly documented as intentionally discarded. + +**BAD** — silently ignoring a result: +```rust +let _ = self.storage.update_meta(&m); +``` + +**BAD** — using `.ok()` without logging: +```rust +let content = fs::read_to_string(&path).ok()?; +``` + +**GOOD** — logging before discarding: +```rust +if let Err(e) = self.storage.update_meta(&m) { + warn!("Failed to update meta for {}: {}", session_id, e); +} +``` + +**GOOD** — propagating with context: +```rust +let content = fs::read_to_string(&path) + .with_context(|| format!("Failed to read transcript for {}", session_id))?; +``` + +Note: `.ok()` is acceptable for truly optional reads (e.g., `read_session_summary_uncached` where the index file may legitimately not exist), but new code should prefer explicit handling. + +### 2. Error Context in anyhow Chains + +When using `anyhow::Result`, errors should include enough context for debugging from logs alone. The daemon runs in the background, so stack traces aren't visible to users. + +**BAD** — bare `?` with no context: +```rust +let meta = self.storage.read_meta(&entry.session_id)?; +``` + +**GOOD** — adding session context: +```rust +let meta = self.storage.read_meta(&entry.session_id) + .with_context(|| format!("reading meta for session {}", entry.session_id))?; +``` + +Focus on new code in `src/sync/`, `src/daemon/`, and `src/hooks/` where errors propagate through multiple layers. + +### 3. thiserror vs anyhow Boundaries + +Domain errors (`AuthError`, etc.) should use `thiserror` with meaningful variants. Generic errors should use `anyhow`. Check that: +- New error types in public module APIs use `thiserror` (like `AuthError` in `src/auth/mod.rs`) +- Internal helper functions use `anyhow::Result` with `.context()` +- No `anyhow::anyhow!("some string")` where a proper error variant would be clearer + +**BAD** — stringly-typed domain error: +```rust +Err(anyhow::anyhow!("not logged in")) +``` + +**GOOD** — typed domain error: +```rust +Err(AuthError::NotLoggedIn) +``` + +### 4. Daemon Resilience + +The daemon must never crash from a single session's error. Check that: +- Loop bodies in `src/sync/poller.rs` and `src/sync/uploader.rs` catch errors per-session and continue processing other sessions +- `tokio::spawn` tasks have error handling at the top level +- Panics are caught at task boundaries (or the code avoids panicking) + +**BAD** — one session error kills the loop: +```rust +for session in sessions { + self.process(session)?; // exits loop on first error +} +``` + +**GOOD** — per-session error handling: +```rust +for session in sessions { + if let Err(e) = self.process(session) { + warn!("Error processing session {}: {}", session.id, e); + } +} +``` + +## Key Files to Check + +- `src/sync/uploader.rs` — upload error handling and retry logic +- `src/sync/poller.rs` — polling loop resilience +- `src/daemon/` — daemon startup and task spawning +- `src/auth/mod.rs` — `AuthError` as the reference pattern for `thiserror` +- `src/hooks/` — hook event processing +- `src/cli/` — CLI-facing error messages (should be user-friendly) + +## Exclusions + +- Existing `.ok()` usage in `read_session_summary_uncached` (intentional for optional data) +- Test code error handling patterns +- Build scripts or tooling diff --git a/.continue/checks/security-auth-review.md b/.continue/checks/security-auth-review.md new file mode 100644 index 0000000..40e5d41 --- /dev/null +++ b/.continue/checks/security-auth-review.md @@ -0,0 +1,99 @@ +--- +name: Security & Auth Review +description: Check token handling, credential storage, secret leakage prevention, and crypto usage for security best practices. +--- + +# Security & Auth Review + +## Context + +Threader handles OAuth tokens (WorkOS device flow), encrypted credential storage (XChaCha20-Poly1305 + Argon2), and transmits user session data to the cloud. The daemon runs continuously in the background, so security issues persist silently. Credentials are stored encrypted at `~/.local/share/threader/daemon/credentials.enc` with machine-ID-derived keys. + +## What to Check + +### 1. Token and Secret Leakage in Logs + +Tokens, credentials, and session content must never appear in log output. The project uses `tracing` for structured logging. + +**BAD** — logging the token value: +```rust +info!("Using token: {}", token); +debug!("Auth response: {:?}", auth_response); +``` + +**GOOD** — logging only metadata: +```rust +debug!("Got auth token (len={})", token.len()); +info!("Auth token refreshed for user {}", creds.user_id); +``` + +Check all `tracing::` calls (`debug!`, `info!`, `warn!`, `error!`) in `src/auth/`, `src/sync/uploader.rs`, and `src/daemon/` for accidental secret logging. Also check that `Debug` derives on structs containing tokens don't leak values (e.g., `Credentials` in `src/auth/mod.rs` derives `Debug`). + +### 2. Credential Storage Security + +Credentials are encrypted with XChaCha20-Poly1305 using a key derived from the machine ID via Argon2. Check that: +- The encryption key is never written to disk or logged +- File permissions are set to `0o600` on Unix after writing (`src/auth/storage.rs:150-154`) +- Nonces are generated from `OsRng` (cryptographically secure), never reused or predictable +- Plaintext credentials are not left in memory longer than necessary (no intermediate temp files) + +**BAD** — writing plaintext credentials: +```rust +fs::write(&path, &json)?; // plaintext on disk +``` + +**GOOD** — encrypt before writing: +```rust +let encrypted = encrypt(json.as_bytes(), &key)?; +fs::write(&path, &encrypted)?; +``` + +### 3. Network Security + +All HTTP requests must use HTTPS. The project uses `reqwest` with `rustls-tls`. + +Check that: +- No hardcoded `http://` URLs (only `https://`) +- Environment variable overrides (`THREADER_CONVEX_SITE_URL`, `THREADER_WORKOS_CLIENT_ID`) don't downgrade to HTTP without warning +- Bearer tokens are sent via `.bearer_auth()`, not in URL query parameters or custom headers +- Response bodies from failed auth requests are not re-exposed to users in a way that leaks server internals + +**BAD** — token in URL: +```rust +let url = format!("{}/api/sessions?token={}", base_url, token); +``` + +**GOOD** — token in Authorization header: +```rust +self.client.post(&url).bearer_auth(token).json(&body).send().await?; +``` + +### 4. Auth Flow Correctness + +The device flow (`src/auth/device_flow.rs`) and token refresh (`src/auth/mod.rs`) must handle edge cases securely: +- Expired tokens must trigger re-auth, not fail silently +- Refresh token rotation: after refresh, the old refresh token must be replaced with the new one +- JWT parsing (`parse_jwt_expiry`) must not trust unverified claims for security decisions beyond caching — currently used only for expiry check, which is appropriate +- The `client_id` is not a secret (it's a public identifier), but API keys or secrets must never be hardcoded + +### 5. Input Validation at System Boundaries + +Data from external sources (Claude Code transcript files, server responses, environment variables) should be validated: +- Transcript JSONL lines should be valid JSON before sending to the server +- Server error responses should not be blindly displayed to users (may contain internal details) +- File paths from `meta.transcript_path` should be validated to prevent path traversal + +## Key Files to Check + +- `src/auth/mod.rs` — token management, JWT parsing, refresh flow +- `src/auth/storage.rs` — encrypted credential storage, key derivation +- `src/auth/device_flow.rs` — OAuth device flow +- `src/sync/uploader.rs` — bearer token usage, HTTPS endpoints +- `src/main.rs` — Sentry DSN and telemetry configuration +- `install.sh` — installation script (curl-pipe-bash pattern) + +## Exclusions + +- Server-side auth validation (lives in `threader-internal`) +- Sentry telemetry data (controlled by `THREADER_NO_TELEMETRY=1`, not a credential concern) +- Public values like `DEFAULT_CLIENT_ID` (WorkOS client IDs are public by design) diff --git a/.continue/checks/sync-correctness.md b/.continue/checks/sync-correctness.md new file mode 100644 index 0000000..3c1898f --- /dev/null +++ b/.continue/checks/sync-correctness.md @@ -0,0 +1,79 @@ +--- +name: Sync Correctness +description: Verify that sync logic preserves the append-only, idempotent, convergence-over-coordination guarantees defined in docs/SYNC.md. +--- + +# Sync Correctness + +## Context + +Threader's daemon replicates Claude Code transcript JSONL files to the cloud. The sync system is built on four principles documented in `docs/SYNC.md`: server-owned watermarks, idempotent writes, reading from the source transcript, and convergence over coordination. Violating any of these can cause data loss, duplicate lines, or silent sync failures — all invisible to the user since the daemon runs in the background. + +## What to Check + +### 1. Source of Truth Violations + +The uploader must always read from the **source transcript** (`meta.transcript_path` — Claude Code's JSONL file), never from the daemon's local copy at `~/.threader/sessions//transcript.jsonl`. The local copy can diverge due to concurrent poller and hook appends. + +**BAD** — reading from local storage: +```rust +let content = self.storage.read_transcript(&entry.session_id)?; +``` + +**GOOD** — reading from Claude Code's source file: +```rust +let content = std::fs::read_to_string(&meta.transcript_path)?; +``` + +Look for any new code in `src/sync/` that reads transcript content from `LocalStorage` instead of `meta.transcript_path`. + +### 2. Idempotency Breaks + +Every upload operation must be safe to retry. Check that: +- `upload_create` remains idempotent (server handles duplicate creates) +- `upload_append` sends `start_line` so the server can deduplicate by line number +- No code assumes "if I sent it, it was received" — network failures between send and ACK must be tolerable +- Queue entries are only removed after a successful server response, not before + +**BAD** — removing from queue before confirming upload: +```rust +self.queue.remove(&path)?; +self.upload(&entry, &token).await?; +``` + +**GOOD** — removing only after success: +```rust +self.upload(&entry, &token).await?; +self.queue.remove(&path)?; +``` + +### 3. Cursor and Watermark Consistency + +The cursor (`CursorTracker`) is an optimization — correctness must not depend on it. Check that: +- If the cursor is ahead of actual synced lines, the system recovers (server's watermark is authoritative) +- If the cursor is behind, duplicate sends are handled (idempotent append) +- Cursor advances only happen after successful queue enqueue, not before + +Review `src/sync/cursor.rs` and `src/sync/poller.rs` for ordering: the pattern should be enqueue first, then advance cursor. + +### 4. Concurrent Access Safety + +The poller and hook handler can both trigger syncs for the same session. Check that: +- `SESSION_SUMMARY_CACHE` (a `Mutex>`) is accessed safely — lock held briefly, no `.unwrap()` on lock +- No TOCTOU races in file reads (e.g., checking file length then reading — the file may have grown) +- Queue operations are atomic or tolerant of partial writes + +## Key Files to Check + +- `src/sync/uploader.rs` — upload logic, source transcript reads, queue processing +- `src/sync/poller.rs` — periodic transcript polling, cursor management +- `src/sync/cursor.rs` — cursor tracking and advancement +- `src/hooks/` — hook handler that enqueues sync operations +- `src/storage/queue.rs` — upload queue operations +- `docs/SYNC.md` — authoritative design principles + +## Exclusions + +- Server-side deduplication logic (lives in `threader-internal`, not this repo) +- Image processing in `src/sync/images.rs` (separate concern) +- Cost calculation in `src/sync/cost.rs` (metadata only, not transcript correctness)