Grows the single-row user_ai_prefs table with five new nullable columns and makes them the server-side fallback layer for AI + reel requests. Resolution order at every consumer: explicit request param → stored pref → current hard default. A missing pref row changes nothing (behavior is byte-identical to today).
default_model, default_backend, default_persona_id — fall through the existing model / backend / persona_id request fields on insight generation (agentic + non-agentic) and all three insight-chat turn variants (sync, stream, async).
reel_span, reel_max_segments — fall through CreateReelRequest.span / .max_segments at reel creation; stored values are validated at write time (day|week|month, positive integer).
PUT /prefs accepts the five fields with the same three-state partial-update semantics as the existing fields (omitted = unchanged, explicit null = clear, value = set); TriState generalized to TriState<T> so the numeric reel_max_segments gets the same treatment. Validation (backend must be consumable by the canonical BackendKind::parse; span must parse via the new MemoriesSpan::from_str) happens before any write — invalid → 400, nothing persisted.
GET /prefs returns the new fields (null when no row).
The reel passive mirror (capture_prefs) now reads the existing row and carries the three AI owner-defaults through unchanged — the upsert overwrites every column, so writing None there would have wiped the owner's defaults on every reel creation. It still mirrors the effective (post-resolution) reel span/segments, so last-used tracking stays idempotent.
Also records ADR-0002 (the .env vs owner-defaults-row config boundary this feature establishes: boot-time config + secrets stay in .env; per-request tuning moves to the DB row; resolution order request param → owner default → hard default; safe-by-default privacy posture for default_backend) and adds the Configuration glossary terms to CONTEXT.md.
Evidence
Migration: 2026-09-27-000000_add_owner_defaults (5 × ADD COLUMN, all nullable) + paired down.sql; schema.rs hand-edited to match (no diesel CLI in this environment).
Tests: 1463/0 (was 1436 at master; +27 new/expanded):
DAO: full-row upsert, overwrite-to-NULL, partial writes covering the new columns.
PUT /prefs: set/clear round-trips incl. the i32 field, omitted-keeps, null-clears-stays-cleared, invalid backend → 400 (nothing written), invalid span → 400 (nothing written), validator unit test (case-insensitive, blank + unknown rejected).
Resolution order: apply_owner_defaults (request-wins, pref-fills, no-row-unchanged, partial pref) and resolve_reel_params (request-wins, pref-fills, no-pref hard defaults, corrupt stored values skipped).
Regression: reel_capture_preserves_owner_defaults — seed owner defaults, run reel capture, assert they survive.
cargo clippy clean, cargo fmt applied.
Two-axis subagent review (fresh-context, per the repo code-review skill):
Spec: all 7 ticket requirements CONFORMANT with file:line evidence — including verification that capture_prefs preserves the owner-default columns (no clobber) and that is_valid_backend delegates to the canonical BackendKind::parse. Noted (accepted): resolution is tested at the pure-function level (apply_owner_defaults, resolve_reel_params) rather than per-route end-to-end (an end-to-end insight test would require a live LLM backend), and corrupt stored reel values fall back to hard defaults by design.
Standards: zero findings — migration shape/comment style, DAO error handling, test patterns (in-memory SQLite, JWT house pattern, #[actix_rt::test]), naming, and CLAUDE.md sync all conform; no new #[allow], no dead code.
Merge Danger
Schema: additive nullable columns on a single-row table; auto-migrates at startup. Down migration drops the five columns (data loss on those columns only). No existing client is affected — new fields are ignored by older clients, and NULL rows behave exactly as before.
Wire shape: GET /prefs gains five null fields (additive); PUT /prefs body gains five optional fields. FileViewer-React / Apollo need no changes to keep working; they gain the server-side fallback for free (Apollo's "SERVER DEFAULT" model row already relies on this path).
Behavior: requests that omit model/backend/persona/span/segments now honor the stored row instead of the hard default. Anyone who has never written the new fields sees zero change.
Rollback: revert the binary + migration down (or leave columns in place — they're inert if the code isn't reading them).
## Summary
Closes #127 — **Prefs: owner defaults (model, backend, persona_id, reel span/length)**.
Grows the single-row `user_ai_prefs` table with five new nullable columns and makes them the server-side fallback layer for AI + reel requests. Resolution order at every consumer: **explicit request param → stored pref → current hard default**. A missing pref row changes nothing (behavior is byte-identical to today).
- `default_model`, `default_backend`, `default_persona_id` — fall through the existing `model` / `backend` / `persona_id` request fields on insight generation (agentic + non-agentic) and all three insight-chat turn variants (sync, stream, async).
- `reel_span`, `reel_max_segments` — fall through `CreateReelRequest.span` / `.max_segments` at reel creation; stored values are validated at write time (`day|week|month`, positive integer).
- `PUT /prefs` accepts the five fields with the same three-state partial-update semantics as the existing fields (omitted = unchanged, explicit null = clear, value = set); `TriState` generalized to `TriState<T>` so the numeric `reel_max_segments` gets the same treatment. Validation (backend must be consumable by the canonical `BackendKind::parse`; span must parse via the new `MemoriesSpan::from_str`) happens **before** any write — invalid → 400, nothing persisted.
- `GET /prefs` returns the new fields (null when no row).
The reel passive mirror (`capture_prefs`) now reads the existing row and carries the three AI owner-defaults through unchanged — the upsert overwrites every column, so writing `None` there would have wiped the owner's defaults on every reel creation. It still mirrors the *effective* (post-resolution) reel span/segments, so last-used tracking stays idempotent.
Also records **ADR-0002** (the `.env` vs owner-defaults-row config boundary this feature establishes: boot-time config + secrets stay in `.env`; per-request tuning moves to the DB row; resolution order request param → owner default → hard default; safe-by-default privacy posture for `default_backend`) and adds the Configuration glossary terms to CONTEXT.md.
## Evidence
- **Migration**: `2026-09-27-000000_add_owner_defaults` (5 × `ADD COLUMN`, all nullable) + paired down.sql; `schema.rs` hand-edited to match (no diesel CLI in this environment).
- **Tests**: 1463/0 (was 1436 at master; +27 new/expanded):
- DAO: full-row upsert, overwrite-to-NULL, partial writes covering the new columns.
- `PUT /prefs`: set/clear round-trips incl. the i32 field, omitted-keeps, null-clears-stays-cleared, invalid backend → 400 (nothing written), invalid span → 400 (nothing written), validator unit test (case-insensitive, blank + unknown rejected).
- Resolution order: `apply_owner_defaults` (request-wins, pref-fills, no-row-unchanged, partial pref) and `resolve_reel_params` (request-wins, pref-fills, no-pref hard defaults, corrupt stored values skipped).
- **Regression**: `reel_capture_preserves_owner_defaults` — seed owner defaults, run reel capture, assert they survive.
- `cargo clippy` clean, `cargo fmt` applied.
- Two-axis subagent review (fresh-context, per the repo `code-review` skill):
- **Spec: all 7 ticket requirements CONFORMANT** with file:line evidence — including verification that `capture_prefs` preserves the owner-default columns (no clobber) and that `is_valid_backend` delegates to the canonical `BackendKind::parse`. Noted (accepted): resolution is tested at the pure-function level (`apply_owner_defaults`, `resolve_reel_params`) rather than per-route end-to-end (an end-to-end insight test would require a live LLM backend), and corrupt stored reel values fall back to hard defaults by design.
- **Standards: zero findings** — migration shape/comment style, DAO error handling, test patterns (in-memory SQLite, JWT house pattern, `#[actix_rt::test]`), naming, and CLAUDE.md sync all conform; no new `#[allow]`, no dead code.
## Merge Danger
- **Schema**: additive nullable columns on a single-row table; auto-migrates at startup. Down migration drops the five columns (data loss on those columns only). No existing client is affected — new fields are ignored by older clients, and NULL rows behave exactly as before.
- **Wire shape**: `GET /prefs` gains five null fields (additive); `PUT /prefs` body gains five optional fields. FileViewer-React / Apollo need no changes to keep working; they gain the server-side fallback for free (Apollo's "SERVER DEFAULT" model row already relies on this path).
- **Behavior**: requests that omit model/backend/persona/span/segments now honor the stored row instead of the hard default. Anyone who has never written the new fields sees zero change.
- Rollback: revert the binary + `migration down` (or leave columns in place — they're inert if the code isn't reading them).
Closes#127.
Grows user_ai_prefs with five nullable owner-default columns and makes
them the server-side fallback layer: request param -> stored pref ->
hard default, at insight generation (agentic + standard), all three
insight-chat turn variants, and reel creation.
- PUT /prefs accepts the fields with the existing three-state partial
update semantics (TriState generalized to TriState<T> for the numeric
reel_max_segments); validation (canonical BackendKind::parse,
MemoriesSpan::from_str) runs before any write.
- GET /prefs returns the new fields.
- capture_prefs reads the existing row and carries the AI owner-defaults
through unchanged (the upsert overwrites all columns), mirroring only
the effective reel span/segments.
- MemoriesSpan gains Display + FromStr so stored spans round-trip
losslessly.
1463/0 tests (+27), clippy clean.
Records the config boundary the #127 owner-default columns establish:
.env = boot-time deployment config + secrets (never DB), user_ai_prefs
single row = per-request owner defaults (hot-tunable, no secrets),
resolution request param -> owner default -> hard default, and the
safe-by-default privacy posture for default_backend. Adds the
Configuration glossary section to CONTEXT.md (owner default,
deployment config, hard default).
Add OpenTelemetry tracing to every LLM client call so an insight/chat
turn is a single trace from HTTP entry to each downstream inference.
- LlmClient trait: thread a &opentelemetry::Context through all 5
methods (generate, chat_with_tools, chat_with_tools_stream,
generate_embeddings, describe_image), matching the DAO convention of
explicit context threading rather than ambient Context::current().
- otel::start_llm_request_span: single shared helper that every
provider (openrouter / llamacpp / ollama) funnels through, so the
span shape is identical: name llm.request, SpanKind::Client,
llm.provider / llm.operation / llm.model.
- Per-method spans in all three clients with the spec's operation enum
(chat | chat_stream | embeddings | describe_image | generate); stream
spans are owned and moved into the async_stream generator so they end
when the response drains, not when headers arrive.
- llm.tokens.input / llm.tokens.output recorded on non-stream and
stream completion; llm.cost_usd from usage.cost (OpenRouter);
http.status_code + error status on failure.
- W3C traceparent injected on outgoing LLM HTTP requests (llama-swap,
Ollama, OpenRouter) gated on span_context.is_valid() so debug
no-op-tracer builds don't send an all-zero parent.
- Turn-level spans (ai.chat.turn.execute, ai.persona_chat.turn.execute)
now carry llm.backend + llm.model; with_entry streaming paths thread
the turn context (turn_cx) to child describe_image / agentic-loop
calls instead of the ambient context.
clippy clean; 690 lib + 775 integration + 4 doc tests green.
Addresses the P2 refactoring follow-ups parked on issue #130:
- otel::with_traceparent(builder, &SpanContext) — one shared W3C
traceparent injector. Replaces the 4-line inline header loop in
llamacpp (3x) and openrouter (3x) and removes ollama's private
inject_traceparent method (4 call sites now use the shared helper).
Takes &SpanContext (not &dyn Span) because the Span trait is not dyn
compatible in opentelemetry 0.31.
- otel::record_http_failure(span, status, message) and
otel::set_http_status(span, status) — collapse the repeated
'set http.status_code attribute + set_status(Status::error)'
boilerplate across all three clients (~10 sites).
Net -45 lines. The remaining if-let-Some(span.as_mut()) guards (token
attributes, parse-failure status) stay inline rather than growing a
SpanHandle wrapper type for a pattern that's already readable.
Good catch — and no, it does not work as intended. Context::current() here is always the empty root context, for three independent reasons:
The codebase threads OTel contexts explicitly and never calls .attach() anywhere. That's the documented intent (llm_client.rs: "thread contexts explicitly rather than relying on ambient Context::current()").
There's no context-aware tokio layer — the deps are only the core otel crates (no opentelemetry-instrumentation-tokio), so nothing propagates a context across .await.
This path runs the turn on a tokio::spawn (chat_turn_stream_inner), so even an attached thread-local wouldn't cross the boundary.
Net effect: the describe_imagellm.request span (and the agentic-loop spans) on the legacy SSE path were orphaned root spans — fresh trace ID, uncorrelated with the HTTP request or turn. They still recorded cost/tokens/status and injected a valid (new-trace) traceparent, just unattached.
Fixed in f850c53: the handler now extracts the request context and threads it down chat_turn_stream → chat_turn_stream_inner → run_streaming_turn → run_bootstrap/continuation_streaming, and the three Context::current() sites use it. Mirrors the non-streaming path, which already threads parent_cx → loop_cx.
Scope heads-up: the same Context::current() placeholder still lives in persona_chat.rs, several spots in insight_generator.rs, and daily_summary_job.rs — those are separate entry points, not this streaming path. I'd rather track those as a focused follow-up than widen this PR. Want me to open an issue for it?
Good catch — and no, it does **not** work as intended. `Context::current()` here is always the empty root context, for three independent reasons:
1. The codebase threads OTel contexts explicitly and **never** calls `.attach()` anywhere. That's the documented intent (`llm_client.rs`: *"thread contexts explicitly rather than relying on ambient `Context::current()`"*).
2. There's no context-aware tokio layer — the deps are only the core otel crates (no `opentelemetry-instrumentation-tokio`), so nothing propagates a context across `.await`.
3. This path runs the turn on a `tokio::spawn` (`chat_turn_stream_inner`), so even an attached thread-local wouldn't cross the boundary.
Net effect: the `describe_image` `llm.request` span (and the agentic-loop spans) on the legacy SSE path were **orphaned root spans** — fresh trace ID, uncorrelated with the HTTP request or turn. They still recorded cost/tokens/status and injected a valid (new-trace) traceparent, just unattached.
**Fixed** in `f850c53`: the handler now extracts the request context and threads it down `chat_turn_stream → chat_turn_stream_inner → run_streaming_turn → run_bootstrap/continuation_streaming`, and the three `Context::current()` sites use it. Mirrors the non-streaming path, which already threads `parent_cx → loop_cx`.
Scope heads-up: the same `Context::current()` placeholder still lives in `persona_chat.rs`, several spots in `insight_generator.rs`, and `daily_summary_job.rs` — those are separate entry points, not this streaming path. I'd rather track those as a focused follow-up than widen this PR. Want me to open an issue for it?
The legacy SSE streaming path (chat_turn_stream -> run_streaming_turn ->
run_bootstrap/continuation_streaming) built its LLM spans from
Context::current(), which is always the empty root context here: the codebase
threads OTel contexts explicitly and never attaches one, there is no
context-aware tokio layer, and the turn runs on a tokio::spawn. So every
llm.request span on that path was an orphaned root span, uncorrelated with the
HTTP request.
Thread the handler's extracted request context down through
chat_turn_stream -> chat_turn_stream_inner -> run_streaming_turn ->
run_bootstrap_streaming / run_continuation_streaming, and use it for the
describe_image and agentic-loop calls. Mirrors the non-streaming path, which
already threads parent_cx -> loop_cx.
Also rename the block-scoped db 'cx' in run_streaming_turn to db_cx so it no
longer shadows the new parameter.
cameron
merged commit da3901c564 into master2026-09-29 01:50:09 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
cameron
Summary
Closes #127 — Prefs: owner defaults (model, backend, persona_id, reel span/length).
Grows the single-row
user_ai_prefstable with five new nullable columns and makes them the server-side fallback layer for AI + reel requests. Resolution order at every consumer: explicit request param → stored pref → current hard default. A missing pref row changes nothing (behavior is byte-identical to today).default_model,default_backend,default_persona_id— fall through the existingmodel/backend/persona_idrequest fields on insight generation (agentic + non-agentic) and all three insight-chat turn variants (sync, stream, async).reel_span,reel_max_segments— fall throughCreateReelRequest.span/.max_segmentsat reel creation; stored values are validated at write time (day|week|month, positive integer).PUT /prefsaccepts the five fields with the same three-state partial-update semantics as the existing fields (omitted = unchanged, explicit null = clear, value = set);TriStategeneralized toTriState<T>so the numericreel_max_segmentsgets the same treatment. Validation (backend must be consumable by the canonicalBackendKind::parse; span must parse via the newMemoriesSpan::from_str) happens before any write — invalid → 400, nothing persisted.GET /prefsreturns the new fields (null when no row).The reel passive mirror (
capture_prefs) now reads the existing row and carries the three AI owner-defaults through unchanged — the upsert overwrites every column, so writingNonethere would have wiped the owner's defaults on every reel creation. It still mirrors the effective (post-resolution) reel span/segments, so last-used tracking stays idempotent.Also records ADR-0002 (the
.envvs owner-defaults-row config boundary this feature establishes: boot-time config + secrets stay in.env; per-request tuning moves to the DB row; resolution order request param → owner default → hard default; safe-by-default privacy posture fordefault_backend) and adds the Configuration glossary terms to CONTEXT.md.Evidence
2026-09-27-000000_add_owner_defaults(5 ×ADD COLUMN, all nullable) + paired down.sql;schema.rshand-edited to match (no diesel CLI in this environment).PUT /prefs: set/clear round-trips incl. the i32 field, omitted-keeps, null-clears-stays-cleared, invalid backend → 400 (nothing written), invalid span → 400 (nothing written), validator unit test (case-insensitive, blank + unknown rejected).apply_owner_defaults(request-wins, pref-fills, no-row-unchanged, partial pref) andresolve_reel_params(request-wins, pref-fills, no-pref hard defaults, corrupt stored values skipped).reel_capture_preserves_owner_defaults— seed owner defaults, run reel capture, assert they survive.cargo clippyclean,cargo fmtapplied.code-reviewskill):capture_prefspreserves the owner-default columns (no clobber) and thatis_valid_backenddelegates to the canonicalBackendKind::parse. Noted (accepted): resolution is tested at the pure-function level (apply_owner_defaults,resolve_reel_params) rather than per-route end-to-end (an end-to-end insight test would require a live LLM backend), and corrupt stored reel values fall back to hard defaults by design.#[actix_rt::test]), naming, and CLAUDE.md sync all conform; no new#[allow], no dead code.Merge Danger
GET /prefsgains five null fields (additive);PUT /prefsbody gains five optional fields. FileViewer-React / Apollo need no changes to keep working; they gain the server-side fallback for free (Apollo's "SERVER DEFAULT" model row already relies on this path).migration down(or leave columns in place — they're inert if the code isn't reading them).@@ -1670,1 +1690,3 @@Some(b64) => match backend.local().describe_image(b64).await {Some(b64) => match backend.local().describe_image(&opentelemetry::Context::current(), b64)any way to know this
Context::curent()will actually work as intended?Good catch — and no, it does not work as intended.
Context::current()here is always the empty root context, for three independent reasons:.attach()anywhere. That's the documented intent (llm_client.rs: "thread contexts explicitly rather than relying on ambientContext::current()").opentelemetry-instrumentation-tokio), so nothing propagates a context across.await.tokio::spawn(chat_turn_stream_inner), so even an attached thread-local wouldn't cross the boundary.Net effect: the
describe_imagellm.requestspan (and the agentic-loop spans) on the legacy SSE path were orphaned root spans — fresh trace ID, uncorrelated with the HTTP request or turn. They still recorded cost/tokens/status and injected a valid (new-trace) traceparent, just unattached.Fixed in
f850c53: the handler now extracts the request context and threads it downchat_turn_stream → chat_turn_stream_inner → run_streaming_turn → run_bootstrap/continuation_streaming, and the threeContext::current()sites use it. Mirrors the non-streaming path, which already threadsparent_cx → loop_cx.Scope heads-up: the same
Context::current()placeholder still lives inpersona_chat.rs, several spots ininsight_generator.rs, anddaily_summary_job.rs— those are separate entry points, not this streaming path. I'd rather track those as a focused follow-up than widen this PR. Want me to open an issue for it?@@ -714,0 +826,4 @@s.set_status(Status::Ok);}Ok((chat_msg, prompt_tokens, completion_tokens))We may want to return cost too, so a client could also report cost if they want.