Compare commits
6 Commits
47062ed207
...
main
| Author | SHA1 | Date | |
|---|---|---|---|
| 484e290d9a | |||
| 573b1840ef | |||
| 466892de97 | |||
| 609ee5aa7e | |||
| 29200e49db | |||
| 038f3cea91 |
@@ -0,0 +1,618 @@
|
||||
# Local LLM efficiency tickets
|
||||
|
||||
Work items for making SimbaRAG's agent answer faster on the local llama-server
|
||||
without losing answer quality. Each ticket is written to be picked up cold: it
|
||||
says what to change, where, why, how to test it, and what "done" means.
|
||||
|
||||
Do the tickets in order. EFF-1 is a prerequisite for measuring EFF-2 and EFF-4,
|
||||
and EFF-3 is the safety net that tells you whether EFF-2 made answers worse.
|
||||
|
||||
---
|
||||
|
||||
## Background: read this first
|
||||
|
||||
### The model server
|
||||
|
||||
- llama-server runs on a separate GPU box at `http://192.168.1.7:8085`
|
||||
(OpenAI-compatible API under `/v1`). The `.env` / `.env.example` addresses
|
||||
are stale; do not trust them.
|
||||
- Model: `Qwen3.8-27B-UD-Q4_K_XL.gguf` (unsloth GGUF) on one RTX 3090 (24 GB),
|
||||
llama.cpp build `b10884` (unsloth fork), vision enabled.
|
||||
- As of 2026-10-12 the server runs with:
|
||||
`--parallel 2` (two slots, 32,768 tokens each), KV cache `q8_0`/`q8_0`,
|
||||
flash attention on, and **MTP speculative decoding**
|
||||
(`--spec-type draft-mtp --spec-draft-n-max 3`).
|
||||
- The server-side config is **not** in this repo (it is a systemd unit on the
|
||||
GPU box). Do not try to change it. Tickets here are app-side only.
|
||||
|
||||
### What we measured (so you understand the goal)
|
||||
|
||||
- Before MTP: ~34 tokens/s generation. After MTP: ~84 tokens/s on tool-call
|
||||
output, ~57 tokens/s on prose.
|
||||
- A real chat turn: 7k-13.5k prompt tokens, ~4 model calls per turn,
|
||||
0.8-3 s prompt processing per call, **~16 s before the first answer
|
||||
token**. Prompt processing is NOT the bottleneck. Generated tokens are:
|
||||
every model call first generates hidden "thinking" tokens, then either a
|
||||
tool call or the answer. With 4 calls per turn, that thinking adds up.
|
||||
- Hard-capping thinking hurts reasoning. Same arithmetic question
|
||||
("cat eats 3/8 cup twice a day, 14-cup bag opened Sept 3, when does it run
|
||||
out?", correct answer **Sept 21**):
|
||||
|
||||
| Request setting | Time thinking | Answer |
|
||||
|---|---|---|
|
||||
| `reasoning_effort: "low"` (what the app sends today) | 5.9 s | Sept 21 (correct) |
|
||||
| + `reasoning_budget_tokens: 64` | 0.97 s | Sept 20 (wrong) |
|
||||
| `chat_template_kwargs: {"enable_thinking": false}` | 0.14 s | Sept 28 (wrong) |
|
||||
|
||||
So the goal is NOT "turn thinking off". It is "turn thinking off or down only
|
||||
on calls where it is useless, and keep it where the model actually reasons".
|
||||
|
||||
### llama-server request fields you will use
|
||||
|
||||
All verified against the live server on build b10884:
|
||||
|
||||
| Field (top level of the JSON request body) | Effect |
|
||||
|---|---|
|
||||
| `reasoning_effort: "low" \| "medium" \| "xhigh"` | Soft hint. The chat template adds a sentence like "Reasoning effort is set to low" to the system prompt. The model usually but not always obeys. The app already sends `"low"` (see `model_kwargs` in `blueprints/conversation/agents.py`). |
|
||||
| `reasoning_budget_tokens: <int>` | **Hard cap** on thinking tokens for this request. When hit, llama-server forces the thinking block closed and the model must answer. (`thinking_budget_tokens` is an alias; use `reasoning_budget_tokens`.) |
|
||||
| `chat_template_kwargs: {"enable_thinking": false}` | No thinking at all for this request. The template pre-fills an empty `<think></think>` block. |
|
||||
|
||||
These are NOT OpenAI API parameters. The Python `openai` client (which
|
||||
LangChain's `ChatOpenAI` uses under the hood) rejects unknown keyword
|
||||
arguments, so they must be sent via **`extra_body`**, e.g.
|
||||
`llama_chat.bind_tools(tools, extra_body={"reasoning_budget_tokens": 512})`.
|
||||
|
||||
Also note: llama-server's `usage` block does **not** include
|
||||
`completion_tokens_details.reasoning_tokens`. That is why `reasoning_tokens`
|
||||
in the app's `stream_request` log line is always `null`. Don't try to "fix"
|
||||
that in the app; it is a server limitation. Use `output_tokens` per call
|
||||
instead (thinking tokens are included in it).
|
||||
|
||||
### How the agent is wired (the parts you will touch)
|
||||
|
||||
- `blueprints/conversation/agents.py`
|
||||
- Near the top: `llama_chat = ChatOpenAI(base_url=..., model_kwargs={"reasoning_effort": "low"}, max_tokens=RESPONSE_RESERVE_TOKENS, ...)`
|
||||
and `openai_fallback = ChatOpenAI(model="gpt-5-mini", ...)`.
|
||||
- `build_agent()` at the bottom of the file. Inside it, `routed_model(messages, config)`
|
||||
runs **once per model call**. It picks the tools, binds them, and returns
|
||||
`prompt | bound_model`. **This is the hook for per-call settings.**
|
||||
- `TOOL_ROUTING_MODE` env var (default `static`):
|
||||
- `static`: every tool the user may use is bound on every call
|
||||
(`router.select_static`). There is no `discover_tool_groups` tool.
|
||||
- `dynamic`: `router.select(messages)` picks tool groups from keywords in
|
||||
the user's message, and the model can call `discover_tool_groups` to
|
||||
unlock more groups on the next call.
|
||||
- Every ticket below must work in **both** modes. Tests should cover both
|
||||
where it matters.
|
||||
- `blueprints/conversation/tool_routing.py`: `ToolRouter`, `DISCOVERY_NAME = "discover_tool_groups"`,
|
||||
`GROUP_TERMS` keyword lists, `select()` / `select_static()`.
|
||||
- `blueprints/conversation/__init__.py`: the streaming endpoint. `_chat_run_stream`
|
||||
→ `event_generator` tracks every model call in the `model_calls` dict
|
||||
(keyed by LangChain `run_id`) from `on_chat_model_start` / `_stream` / `_end`
|
||||
events, and `_log_summary()` writes one JSON log line with
|
||||
`"event": "stream_request"` per user turn. `_model_call_summary()` builds the
|
||||
per-call dict (`prefill_ms`, `ms`, `prompt_tokens`, `output_tokens`, ...).
|
||||
- `blueprints/conversation/context.py`: prompt budgeting (`CONTEXT_WINDOW_TOKENS`,
|
||||
`PROMPT_BUDGET_TOKENS`, `RESPONSE_RESERVE_TOKENS=4096`). You should not need
|
||||
to change it.
|
||||
|
||||
### The message sequence inside one user turn
|
||||
|
||||
LangGraph's ReAct agent loops: model call → (tools run) → model call → ...
|
||||
until the model answers without tool calls. The `messages` list that
|
||||
`routed_model` receives grows like this:
|
||||
|
||||
```
|
||||
... earlier turns ...
|
||||
HumanMessage("how much did we spend on groceries last month?") <- call 1 sees this as the last message
|
||||
AIMessage(tool_calls=[ynab_spending_report(...)]) <- call 1 output
|
||||
ToolMessage(name="ynab_spending_report", content="{...}") <- call 2 sees this as the last message
|
||||
AIMessage("You spent $612 on groceries...") <- call 2 output (final answer)
|
||||
```
|
||||
|
||||
In `dynamic` mode there can be a discovery hop:
|
||||
|
||||
```
|
||||
HumanMessage("...")
|
||||
AIMessage(tool_calls=[discover_tool_groups(groups=["finance"])])
|
||||
ToolMessage(name="discover_tool_groups", content='{"enabled_tool_groups": ["finance"], ...}')
|
||||
AIMessage(tool_calls=[ynab_spending_report(...)]) <- this call needs no real thinking
|
||||
ToolMessage(name="ynab_spending_report", ...)
|
||||
AIMessage("final answer")
|
||||
```
|
||||
|
||||
The model can also make **several tool calls in one AIMessage** (parallel
|
||||
calls), which produces several `ToolMessage`s in a row. `ToolMessage.name` is
|
||||
set to the tool's name by LangChain/LangGraph for both success and error
|
||||
results. `ToolMessage.status` is `"success"` or `"error"`.
|
||||
|
||||
### Ground rules for every ticket
|
||||
|
||||
1. **One ticket = one branch = one PR.** Branch from up-to-date `main`, e.g.
|
||||
`git checkout main && git pull && git checkout -b feat/eff-1-call-kind-logging`.
|
||||
2. Line numbers drift. Find code by function/constant name, not by line number.
|
||||
3. Match surrounding style: plain functions, short docstrings, comments only
|
||||
where the "why" isn't obvious. Don't reformat unrelated code.
|
||||
4. Run the unit tests before pushing:
|
||||
`.venv/bin/pytest tests/unit -q` (or `make test`, which needs `uv`).
|
||||
All tests must pass, including ones you didn't touch.
|
||||
5. If you add an environment variable you must add it to **all four** of:
|
||||
`.env.example`, `docker-compose.yml` (the `environment:` list of the
|
||||
raggr services), `config/validation.py` (defaults/validation), and the
|
||||
real `.env` if present. Prefer module constants unless the ticket says
|
||||
otherwise; fewer knobs is better.
|
||||
6. Do not write DB migrations by hand. None of these tickets should need a
|
||||
migration; if you think one does, stop and ask.
|
||||
7. PRs go to Gitea, not GitHub: `git push -u origin <branch>` then
|
||||
`tea pr create -t "title" -d "body"`.
|
||||
8. Never point tests at the live llama-server. Unit tests must run offline.
|
||||
Only EFF-3's script talks to the real server, and it is not a pytest test.
|
||||
9. Don't commit `.DS_Store`, `chroma_db/`, `chromadb/`, `.planning/`.
|
||||
10. **You probably cannot reach the llama-server or production.** It lives on
|
||||
Ryan's home LAN (`192.168.1.7`), and production logs/DB are on his server.
|
||||
Don't try to work around that, and don't block on it. Every step below
|
||||
that needs the live server or prod data is marked **[Ryan runs]**: write
|
||||
the code so Ryan can run that step with one copy-pasteable command, put
|
||||
that exact command in the PR description under a "For Ryan to run"
|
||||
heading, and finish the rest of the ticket. If you *can* reach
|
||||
`http://192.168.1.7:8085/health` (e.g. running locally on his network),
|
||||
you may run those steps yourself; they only generate text and change
|
||||
nothing on the server.
|
||||
|
||||
---
|
||||
|
||||
## EFF-1: Classify each model call and log the classification
|
||||
|
||||
**Size:** small. **Depends on:** nothing. **Blocks:** EFF-2, EFF-4.
|
||||
|
||||
### Why
|
||||
|
||||
We want to know where the generated tokens go: is it the first call of a turn,
|
||||
the "now call the tool you just unlocked" call after `discover_tool_groups`,
|
||||
or the call that writes the answer from tool results? Today the
|
||||
`stream_request` log has per-call `output_tokens` and `ms` but nothing says
|
||||
which kind of call each one was. EFF-2 also needs this exact classification to
|
||||
pick thinking settings, so it lives in one shared function.
|
||||
|
||||
### What to build
|
||||
|
||||
1. **New module** `blueprints/conversation/thinking.py` with:
|
||||
|
||||
```python
|
||||
"""Per-call classification used to size the model's hidden thinking."""
|
||||
|
||||
from langchain_core.messages import HumanMessage, ToolMessage
|
||||
|
||||
from blueprints.conversation.tool_routing import DISCOVERY_NAME
|
||||
|
||||
FIRST = "first" # last message is the user's question
|
||||
AFTER_DISCOVERY = "after_discovery" # only discover_tool_groups results since the model last spoke
|
||||
AFTER_TOOLS = "after_tools" # real tool results to read (or anything else)
|
||||
|
||||
|
||||
def classify_call(messages) -> str:
|
||||
...
|
||||
```
|
||||
|
||||
Rules, in this order:
|
||||
- Collect the **trailing run** of `ToolMessage`s at the end of `messages`
|
||||
(walk backwards from the end while `isinstance(m, ToolMessage)`; stop at
|
||||
the first non-ToolMessage).
|
||||
- If that run is non-empty **and every** message in it has
|
||||
`m.name == DISCOVERY_NAME` **and** `m.status == "success"` → return
|
||||
`AFTER_DISCOVERY`.
|
||||
- Mixed run (discovery + a real tool in the same parallel batch) →
|
||||
`AFTER_TOOLS`, because there is real data to read.
|
||||
- A discovery result with `status == "error"` → `AFTER_TOOLS` (something
|
||||
went wrong; let the model think).
|
||||
- Else, if `messages` is non-empty and `messages[-1]` is a `HumanMessage`
|
||||
→ `FIRST`.
|
||||
- Else → `AFTER_TOOLS`. This is the safe default (it gets the most
|
||||
thinking in EFF-2), so anything unexpected, including an empty list,
|
||||
lands here.
|
||||
- `messages` may contain a `SystemMessage` at the start; it does not matter
|
||||
for these rules. `messages` might also be passed as a LangGraph state
|
||||
dict `{"messages": [...]}` in some code paths. Look at how
|
||||
`_build_prompt` in `agents.py` handles that
|
||||
(`state["messages"] if isinstance(state, dict) else state`) and do the
|
||||
same at the top of `classify_call`.
|
||||
|
||||
2. **Attach the classification to each model call** in `routed_model`
|
||||
(`blueprints/conversation/agents.py`, inside `build_agent`):
|
||||
- Compute `kind = classify_call(messages)`.
|
||||
- Attach it as LangChain run metadata so it shows up in the streaming
|
||||
events: wrap the returned chain with
|
||||
`.with_config(metadata={"call_kind": kind})`. Example:
|
||||
|
||||
```python
|
||||
chain = RunnableLambda(partial(_build_prompt, tool_schemas=schemas)) | bound
|
||||
return chain.with_config(metadata={"call_kind": kind})
|
||||
```
|
||||
|
||||
LangChain merges config metadata into child runs, so the
|
||||
`on_chat_model_start` event for this call should carry
|
||||
`event["metadata"]["call_kind"]`. **Verify this** in a test (see below);
|
||||
if metadata does not propagate in the installed LangChain version, attach
|
||||
it to `bound` instead (`bound.with_config(...)`), and re-check.
|
||||
|
||||
3. **Record it in the log line** in `blueprints/conversation/__init__.py`:
|
||||
- In the `on_chat_model_start` branch where `model_calls[event["run_id"]] = {...}`
|
||||
is created, add `"call_kind": metadata.get("call_kind")`.
|
||||
- In `_model_call_summary()`, add `"call_kind": call.get("call_kind")` to
|
||||
the returned dict.
|
||||
- Do not change any other field names in the log line; dashboards or
|
||||
scripts may read them.
|
||||
|
||||
### Tests (new file `tests/unit/test_thinking.py`)
|
||||
|
||||
Write a pure-function test for each rule. Build messages with
|
||||
`HumanMessage`, `AIMessage(content="", tool_calls=[{"id": "c1", "name": "...", "args": {}}])`,
|
||||
`ToolMessage(content="...", tool_call_id="c1", name="...", status="success")`.
|
||||
|
||||
- `[HumanMessage]` → `FIRST`.
|
||||
- `[Human, AI(tool_call discover), Tool(name=discover, success)]` → `AFTER_DISCOVERY`.
|
||||
- `[Human, AI(discover + ynab), Tool(discover), Tool(ynab)]` → `AFTER_TOOLS` (mixed parallel batch).
|
||||
- `[Human, AI(discover), Tool(discover, status="error")]` → `AFTER_TOOLS`.
|
||||
- `[Human, AI(ynab), Tool(ynab)]` → `AFTER_TOOLS`.
|
||||
- Two discovery results in a row, both success → `AFTER_DISCOVERY`.
|
||||
- An older discovery earlier in the turn but the last messages are a real
|
||||
tool result → `AFTER_TOOLS` (only the trailing run counts).
|
||||
- A previous turn ended in a tool result, then a new `HumanMessage` → `FIRST`.
|
||||
- `[]` → `AFTER_TOOLS`.
|
||||
- The state-dict form `{"messages": [HumanMessage(...)]}` → `FIRST`.
|
||||
|
||||
Plus one wiring test that the metadata reaches the streaming events. Look at
|
||||
`tests/unit/test_tool_routing.py`: `import_agents()` imports `agents.py` with
|
||||
all external services stubbed, and
|
||||
`test_real_graph_binds_discovered_schemas_and_accounts_same_subset` builds the
|
||||
real graph with a `FakeModel`. Copy that pattern: build the agent, run
|
||||
`agent.astream_events(..., version="v2")` (or whatever the app uses; check
|
||||
`_chat_run_stream`), collect `on_chat_model_start` events, and assert their
|
||||
`metadata["call_kind"]` values are `["first", "after_discovery", ...]` for a
|
||||
dynamic-mode run. If the `FakeModel` there is a plain `RunnableLambda` (not a
|
||||
chat model), you may not get `on_chat_model_start` events at all. In that
|
||||
case use `langchain_core.language_models.fake_chat_models.GenericFakeChatModel`
|
||||
(or `FakeMessagesListChatModel`) with a `bind_tools` that returns itself, so
|
||||
real chat-model events fire. Keep the test offline.
|
||||
|
||||
### Done when
|
||||
|
||||
- `classify_call` exists with the rules above and the tests pass.
|
||||
- **[Ryan runs]** After deploying, a `stream_request` log line has a
|
||||
`call_kind` on every entry in `model_calls`:
|
||||
`make logs | grep stream_request | tail -5`.
|
||||
- No behavior change for users (this ticket only adds logging).
|
||||
|
||||
---
|
||||
|
||||
## EFF-2: Size thinking per call kind (the actual speedup)
|
||||
|
||||
**Size:** small-medium. **Depends on:** EFF-1 (uses `classify_call`).
|
||||
Ideally EFF-3 exists so you can measure before/after.
|
||||
|
||||
### Why
|
||||
|
||||
Thinking is the biggest chunk of time before the first answer token, but the
|
||||
experiment in the Background section shows that killing it everywhere makes
|
||||
math and date answers wrong. Some calls don't need it:
|
||||
|
||||
- **`AFTER_DISCOVERY`** (dynamic mode only): the model just unlocked a tool
|
||||
group; all it has to do now is call the tool. Thinking here is waste.
|
||||
- **`FIRST`**: usually "pick a tool and fill in arguments", but sometimes the
|
||||
model answers directly (chit-chat, or a math question like the cat-food one)
|
||||
with no tool. So we cap it moderately instead of turning it off.
|
||||
- **`AFTER_TOOLS`**: the model reads tool results and writes the answer
|
||||
(YNAB totals, comparing dates). This is where reasoning matters. We only
|
||||
add a generous cap so one runaway call can't burn the whole 4,096-token
|
||||
`max_tokens` on thinking and leave nothing for the answer.
|
||||
|
||||
### What to build
|
||||
|
||||
1. In `blueprints/conversation/thinking.py` (from EFF-1), add:
|
||||
|
||||
```python
|
||||
FIRST_CALL_THINKING_BUDGET = 512
|
||||
AFTER_TOOLS_THINKING_BUDGET = 1024
|
||||
|
||||
|
||||
def thinking_options(kind: str) -> dict:
|
||||
"""llama-server request fields (sent via extra_body) for one call kind."""
|
||||
if kind == AFTER_DISCOVERY:
|
||||
return {"chat_template_kwargs": {"enable_thinking": False}}
|
||||
if kind == FIRST:
|
||||
return {"reasoning_budget_tokens": FIRST_CALL_THINKING_BUDGET}
|
||||
return {"reasoning_budget_tokens": AFTER_TOOLS_THINKING_BUDGET}
|
||||
```
|
||||
|
||||
Keep these as module constants, not environment variables (see ground rule 5).
|
||||
|
||||
2. In `routed_model` (`agents.py`, inside `build_agent`), pass the options to
|
||||
the **llama** model only:
|
||||
|
||||
```python
|
||||
kind = classify_call(messages) # already added in EFF-1
|
||||
fallback = openai_fallback.bind_tools(selected)
|
||||
bound = (
|
||||
llama_chat.bind_tools(selected, extra_body=thinking_options(kind)).with_fallbacks([fallback])
|
||||
if llama_chat
|
||||
else fallback
|
||||
)
|
||||
```
|
||||
|
||||
- **Do not** pass `extra_body` to `openai_fallback`. OpenAI's API rejects
|
||||
unknown fields like `reasoning_budget_tokens`, and the fallback is what
|
||||
saves us when the GPU box is down.
|
||||
- Keep the existing `model_kwargs={"reasoning_effort": "low"}` on
|
||||
`llama_chat`. The soft hint and the hard cap work together.
|
||||
- `bind_tools(tools, **kwargs)` forwards extra kwargs into the request.
|
||||
Confirm with a test (below) that `extra_body` ends up in the bound
|
||||
kwargs. If `ChatOpenAI` in the installed version does not accept
|
||||
`extra_body` through `bind_tools`, use
|
||||
`llama_chat.bind_tools(selected).bind(extra_body=...)`.
|
||||
|
||||
3. Add the options to the log line so we can correlate: in `routed_model`,
|
||||
extend the EFF-1 metadata to
|
||||
`metadata={"call_kind": kind, "thinking": thinking_options(kind)}` and copy
|
||||
`"thinking"` into `model_calls[...]` / `_model_call_summary()` the same way
|
||||
as `call_kind`. (If the fallback model handled the call, the field is still
|
||||
logged; that's fine, `provider` tells you which model ran.)
|
||||
|
||||
### Things that will break and how to fix them
|
||||
|
||||
- `tests/unit/test_tool_routing.py` has fake models with
|
||||
`def bind_tools(self, tools):` (two places). They will now get an
|
||||
unexpected `extra_body=` keyword and raise `TypeError`. Change those fakes to
|
||||
`def bind_tools(self, tools, **kwargs):`. Don't change what the tests assert.
|
||||
- If you used `.bind(extra_body=...)` instead, the fakes need a `.bind`
|
||||
method, or make `bind_tools` return a real `Runnable` (they currently return
|
||||
`RunnableLambda`, which has `.bind`). Check before assuming.
|
||||
|
||||
### Tests (add to `tests/unit/test_thinking.py`)
|
||||
|
||||
- `thinking_options(FIRST) == {"reasoning_budget_tokens": 512}`.
|
||||
- `thinking_options(AFTER_DISCOVERY) == {"chat_template_kwargs": {"enable_thinking": False}}`.
|
||||
- `thinking_options(AFTER_TOOLS) == {"reasoning_budget_tokens": 1024}`.
|
||||
- `thinking_options("something unknown") == {"reasoning_budget_tokens": 1024}`
|
||||
(safe default).
|
||||
- Wiring test, modeled on `test_real_graph_binds_discovered_schemas_and_accounts_same_subset`:
|
||||
a fake llama model whose `bind_tools(self, tools, **kwargs)` records
|
||||
`kwargs`. Run a dynamic-mode turn that does discovery → tool → answer and
|
||||
assert the recorded `extra_body` values were, in order:
|
||||
`{"reasoning_budget_tokens": 512}`, `{"chat_template_kwargs": {"enable_thinking": False}}`,
|
||||
`{"reasoning_budget_tokens": 1024}`.
|
||||
- Same wiring test in static mode (no discovery): expect `512` then `1024`.
|
||||
- Fallback test: with the primary fake set to fail, assert the fallback
|
||||
fake's `bind_tools` was **never** given `extra_body`.
|
||||
|
||||
### Manual check against the real server (**[Ryan runs]**, optional, read-only, not a test)
|
||||
|
||||
You can confirm llama-server honors the fields with curl. This only generates
|
||||
text; it changes nothing on the server:
|
||||
|
||||
```bash
|
||||
curl -s http://192.168.1.7:8085/v1/chat/completions -H 'content-type: application/json' -d '{
|
||||
"model": "qwen3.8-27b", "max_tokens": 1024, "reasoning_effort": "low",
|
||||
"reasoning_budget_tokens": 64,
|
||||
"messages": [{"role": "user", "content": "What is 17*23?"}]
|
||||
}' | python3 -c 'import json,sys; d=json.load(sys.stdin); m=d["choices"][0]["message"]; print(len(m.get("reasoning_content") or ""), d["timings"]["predicted_n"], m["content"])'
|
||||
```
|
||||
|
||||
A small `reasoning_content` length and `predicted_n` around the budget means
|
||||
the cap works.
|
||||
|
||||
### Done when
|
||||
|
||||
- Tests above pass, and the whole unit suite passes.
|
||||
- **[Ryan runs]** EFF-3's evaluation shows: median time-to-first-answer-token lower than
|
||||
before, **no drop** in tool-choice accuracy, **zero** malformed tool calls,
|
||||
and no new wrong answers on the arithmetic/date cases. If the arithmetic
|
||||
cases regress, raise `FIRST_CALL_THINKING_BUDGET` (try 1024) before anything
|
||||
else, and say so in the PR.
|
||||
- The PR description includes the exact commands for Ryan to produce a
|
||||
before/after comparison: EFF-3 run on `main` with `--label before`, then on
|
||||
this branch with `--label after`, then the compare command. If EFF-3 isn't
|
||||
merged yet, instead give him the log query for ~10 real chats (`ttft_ms` and
|
||||
per-call `output_tokens` from `stream_request` lines, grouped by
|
||||
`call_kind`). Ryan merges only after those numbers look right, so don't
|
||||
merge it yourself.
|
||||
|
||||
---
|
||||
|
||||
## EFF-3: Model-level A/B evaluation script
|
||||
|
||||
**Size:** medium. **Depends on:** nothing (but EFF-1/2 make it more useful).
|
||||
|
||||
### Why
|
||||
|
||||
`scripts/evaluate_context.py` only tests history trimming. It never calls a
|
||||
model, so it can't tell us whether a model or server change made answers
|
||||
slower or worse. We need a repeatable script that sends realistic agent
|
||||
requests to whatever `LLAMA_SERVER_URL` points at and reports speed and
|
||||
correctness, so any change (EFF-2, new llama-server flags, a different model)
|
||||
can be judged with numbers.
|
||||
|
||||
### What to build
|
||||
|
||||
1. **Fixture file** `tests/fixtures/model_eval/cases.json`: a list of cases.
|
||||
Each case is one **single model call** (not a full agent loop; that would
|
||||
need the DB, Paperless, YNAB, etc.). Shape:
|
||||
|
||||
```json
|
||||
{
|
||||
"name": "ynab_groceries_last_month",
|
||||
"kind": "tool_choice",
|
||||
"messages": [
|
||||
{"role": "user", "content": "How much did we spend on groceries last month?"}
|
||||
],
|
||||
"expect_tool": "ynab_spending_report",
|
||||
"expect_args_subset": {"category": "Groceries"}
|
||||
}
|
||||
```
|
||||
|
||||
Three kinds of case:
|
||||
- `"tool_choice"`: the user message alone; pass if the model's first
|
||||
response calls `expect_tool`, and `expect_args_subset` (optional) keys
|
||||
match the call's arguments (string compare, case-insensitive).
|
||||
- `"answer"`: messages include a canned assistant tool call **and** a canned
|
||||
tool result (role `"tool"`, realistic JSON copied from a real tool's
|
||||
output shape; look at the YNAB tools in `blueprints/ynab/tools.py` and
|
||||
the artifact shapes in `tests/fixtures/context/scenarios.json`); pass if
|
||||
the final answer text contains every string in `expect_contains`
|
||||
(case-insensitive) and calls no tool.
|
||||
- `"direct"`: no tool should be called; pass if the answer contains
|
||||
`expect_contains`. Include the cat-food question from the Background
|
||||
section (`expect_contains: ["21"]`; the correct date is Sept 21).
|
||||
|
||||
Write **at least 25 cases** covering: pets/Simba (`simba_search`,
|
||||
`simbakit_*`), meals (`mealie_*`), notes/journal (`obsidian_*`,
|
||||
`journal_*`), YNAB spending/balances (`ynab_*`), restaurants (`beli_*`),
|
||||
web search, date questions ("what's on Friday"), 2-3 follow-up questions
|
||||
that only make sense with a previous turn ("what about last month?"; put
|
||||
the previous turn in `messages`), at least 3 `"answer"` cases with
|
||||
arithmetic over tool results, and 2 `"direct"` reasoning questions. Use
|
||||
the real tool names: get them from `agents.py` (`@tool` functions) and
|
||||
`blueprints/ynab/tools.py`. No real personal data in fixtures; invent
|
||||
plausible numbers.
|
||||
|
||||
2. **Script** `scripts/evaluate_model.py`, run as
|
||||
`.venv/bin/python -m scripts.evaluate_model [--cases PATH] [--repeat N] [--label NAME] [--out results.json]`:
|
||||
- Reads `LLAMA_SERVER_URL` and `LLAMA_MODEL_NAME` from the environment
|
||||
(`load_dotenv()` like the rest of the app). Allow `--base-url` to
|
||||
override, because the `.env` value is stale; the real server is
|
||||
`http://192.168.1.7:8085/v1`.
|
||||
- For the system prompt and tool schemas, use the **real ones** so the
|
||||
eval matches production: `SIMBA_SYSTEM_PROMPT` from
|
||||
`blueprints/conversation/prompts.py`, and tool schemas via
|
||||
`convert_to_openai_tool(t)` for the tools in `agents.tools` (plus YNAB
|
||||
tools if they're in that list). Importing `agents.py` initializes
|
||||
services; use the same stubbing trick as `import_agents()` in
|
||||
`tests/unit/test_tool_routing.py` so the import works offline. Bind all
|
||||
tools (static mode) unless `--routing dynamic` is given, in which case
|
||||
use `ToolRouter(...).select(messages)` to pick them like production does.
|
||||
- If `blueprints/conversation/thinking.py` exists (EFF-2 merged), apply
|
||||
`thinking_options(classify_call(...))` per case so the eval reflects
|
||||
production; add `--no-thinking-policy` to compare against today's
|
||||
behavior.
|
||||
- Call the server with the plain `openai` client (`OpenAI(base_url=..., api_key="not-needed")`),
|
||||
**streaming**, with `stream_options={"include_usage": True}`,
|
||||
`reasoning_effort="low"`, `max_tokens=4096`, and any thinking fields in
|
||||
`extra_body`. Streaming lets you measure:
|
||||
- `ttft_ms`: time from request to the first **content or tool-call**
|
||||
delta (ignore deltas that only carry `reasoning_content`).
|
||||
- `total_ms`, `completion_tokens` and `prompt_tokens` (from `usage`).
|
||||
- llama-server also sends a non-standard `timings` object on the final
|
||||
chunk with `predicted_per_second`, `prompt_per_second`, `draft_n`,
|
||||
`draft_n_accepted`. Read it from the raw chunk
|
||||
(`chunk.model_extra.get("timings")` with the openai client) and record
|
||||
it if present; don't fail if absent.
|
||||
- Grade each case per its `kind` (rules above). A tool call whose
|
||||
arguments aren't valid JSON counts as **malformed**: record it
|
||||
separately, it's the worst failure.
|
||||
- Output: a table to stdout (one row per case: name, pass/fail, ttft_ms,
|
||||
total_ms, completion_tokens, tok/s, draft acceptance) and a summary:
|
||||
pass rate per kind, malformed count, median and p90 of ttft_ms and
|
||||
total_ms, median completion_tokens. With `--out`, write all raw
|
||||
results plus the summary as JSON, including `--label`, the base URL,
|
||||
the model name, and a timestamp.
|
||||
- `--repeat N` runs each case N times (default 1). Sampling is random
|
||||
(server default temperature 1.0), so use `--repeat 3` for real
|
||||
comparisons and report pass rate as passes/(cases×N).
|
||||
- Add `scripts/compare_model_eval.py a.json b.json` (or a `--compare`
|
||||
flag) that prints the two summaries side by side with deltas.
|
||||
- Exit code 0 always (it's a report, not a gate), except exit 2 on
|
||||
connection errors.
|
||||
|
||||
3. **Docs:** add a short section to `docs/efficiency/README.md` (create it)
|
||||
explaining how to run it, with an example command and how to read the
|
||||
output. One paragraph plus commands is enough.
|
||||
|
||||
### Tests
|
||||
|
||||
The script itself talks to a live server, so it is **not** a unit test. But
|
||||
add `tests/unit/test_model_eval.py` for the pure parts:
|
||||
- The grading function for each kind, including malformed JSON arguments and
|
||||
case-insensitive substring matching.
|
||||
- Loading `cases.json` validates required keys per kind (fail fast with a
|
||||
clear message naming the bad case).
|
||||
- The summary math (median/p90) on a small hand-made list.
|
||||
Keep these offline: no `OpenAI` client construction in tests.
|
||||
|
||||
### Done when
|
||||
|
||||
- The unit tests pass.
|
||||
- The script runs end to end against a **fake** server so you can prove it
|
||||
works without the LAN: add `--base-url` support and test it manually
|
||||
against a tiny local stub (a ~40-line `http.server` or `aiohttp` script
|
||||
that returns a canned streaming chat-completion with one tool call, a
|
||||
`usage` chunk and a `timings` object). Don't commit the stub unless it's
|
||||
clean enough to live in `tests/fixtures/model_eval/` and is used by a test.
|
||||
- **[Ryan runs]** The PR description ends with the real command for Ryan:
|
||||
`.venv/bin/python -m scripts.evaluate_model --base-url http://192.168.1.7:8085/v1 --repeat 3 --label baseline --out /tmp/baseline.json`
|
||||
|
||||
---
|
||||
|
||||
## EFF-4: Fewer discovery round-trips in dynamic routing mode
|
||||
|
||||
**Size:** small. **Depends on:** EFF-1 (to measure). **Only matters if
|
||||
production runs `TOOL_ROUTING_MODE=dynamic`.** You can't see the production
|
||||
`.env`; ask Ryan which mode production uses before starting. If production is `static`, skip this
|
||||
ticket and note that in the PR list.
|
||||
|
||||
### Why
|
||||
|
||||
Every `discover_tool_groups` call is a full extra model round-trip: prompt
|
||||
processing + generated tokens + the tool call. And because the template puts
|
||||
the tool definitions at the very top of the prompt, the next call has a
|
||||
different tool list, so llama-server's prompt cache misses and it reprocesses
|
||||
the whole prompt. Even with EFF-2 making that call cheaper, it's cheapest not
|
||||
to make it. Most discovery calls happen because the user's words didn't match
|
||||
`GROUP_TERMS` in `tool_routing.py`.
|
||||
|
||||
### What to do
|
||||
|
||||
1. **Collect data first. [Ryan runs]** After EFF-1 is deployed for a few days, pull the
|
||||
user messages that led to discovery. The `stream_request` log has
|
||||
`tools` (tool spans, with names) and `model_calls` (with `call_kind`).
|
||||
A turn where a `discover_tool_groups` tool span appears is a miss. The
|
||||
user's message text is in the DB (`conversation` messages), matched by
|
||||
the `conv` id in the log line. Write a small script (Ryan runs it on the
|
||||
server; commit it under `scripts/` only if it's clean and reusable) that
|
||||
lists, per missed turn: the
|
||||
user message and the groups the model asked for.
|
||||
2. For each miss, add the missing everyday words to the right group's string
|
||||
in `GROUP_TERMS`. Example: if "what did the paycheck cover this month?"
|
||||
triggered discovery of `finance`, add `paycheck paychecks` to the
|
||||
`finance` string. Before adding a word, ask: "would this word show up in
|
||||
questions that have nothing to do with this group?" If yes, don't add it,
|
||||
because extra tools on every call cost prompt tokens and confuse the model.
|
||||
Rules of thumb:
|
||||
- Add nouns and verbs specific to the domain ("groceries", "paycheck",
|
||||
"litter"); avoid generic words ("left", "show", "get", "much").
|
||||
- A word can be in two groups if it really belongs to both.
|
||||
- Remember matching is on whole lowercase words (`words()` splits on
|
||||
non-alphanumerics), so add plural forms separately ("bill bills").
|
||||
3. Add a test per new word family in `tests/unit/test_tool_routing.py` in the
|
||||
existing style: `names(router, [HumanMessage(content="<the real user phrasing>")])`
|
||||
includes the expected tool.
|
||||
|
||||
### Done when
|
||||
|
||||
- Every miss you collected now routes without discovery (tests prove it).
|
||||
- No existing routing test changed its expectations.
|
||||
- PR description lists the misses found and the words added.
|
||||
|
||||
---
|
||||
|
||||
## Manual / ops follow-ups (for Ryan, not for Codex)
|
||||
|
||||
These need the GPU box and are listed so they aren't forgotten:
|
||||
|
||||
- Try `--spec-draft-n-max 4` (tool-call output accepted ~92% of drafts at 3;
|
||||
depth 4 sometimes wins on JSON-heavy output). Compare with EFF-3,
|
||||
`--repeat 3`.
|
||||
- Try `--spec-type draft-mtp,ngram-mod` (n-gram drafting could help when the
|
||||
model copies IDs from tool results into arguments). Unverified; A/B with
|
||||
EFF-3.
|
||||
- Record the final llama-server unit (flags and why) in `docs/deployment.md`
|
||||
so the server config isn't only on the GPU box.
|
||||
- Old unit is backed up at `/etc/systemd/system/llama-server.service.bak`.
|
||||
@@ -0,0 +1,789 @@
|
||||
# Immich MCP: tickets
|
||||
|
||||
Source of truth: [`design.md`](design.md). Section references like "§7.2"
|
||||
point there. When a ticket and the design disagree, the design wins: stop
|
||||
and ask the conductor rather than guessing.
|
||||
|
||||
Line numbers are from `main` at `bae2e75` (2026-10-13). They will drift, so
|
||||
search for the quoted code when they don't match.
|
||||
|
||||
---
|
||||
|
||||
## 1. How this work is run (Herdr rules)
|
||||
|
||||
All of these tickets are built with the **Herdr fan-out workflow** (`/herdr-fanout`).
|
||||
The roles do not change from ticket to ticket.
|
||||
|
||||
### Roles
|
||||
|
||||
- **Conductor pane** (the main Claude session, normally Opus). It plans, hands
|
||||
out work, reviews, and ships. It does **not** write the feature code for
|
||||
worker tickets. The exceptions are tickets marked *Owner: conductor*, and a
|
||||
second-round review that still finds a real bug, which the conductor fixes directly.
|
||||
- **Worker panes** (sub panes, one per ticket, smaller model:
|
||||
`claude --model sonnet`). Each one implements exactly one ticket in its own git
|
||||
worktree and then reports back. Workers never ship.
|
||||
|
||||
### Conductor duties, in order
|
||||
|
||||
1. **Preconditions.** `test "$HERDR_ENV" = 1`; read `CLAUDE.md`; `git fetch`; note
|
||||
`origin/main`. Pick each ticket's **base branch**: `main` once its
|
||||
dependencies are merged. A ticket is never stacked on an unmerged branch
|
||||
unless this file says so.
|
||||
2. **Plan file per ticket.** Copy the ticket section below into
|
||||
`<scratchpad>/plans/<ticket-id>.md`, add the current line anchors, and paste
|
||||
the **worker ground rules** (below) at the bottom.
|
||||
3. **Worktree + pane per ticket.**
|
||||
```bash
|
||||
git worktree add -b <branch> <scratchpad>/wt-<ticket-id> origin/main
|
||||
ln -s ~/Programs/raggr/.venv <scratchpad>/wt-<ticket-id>/.venv
|
||||
ln -s ~/Programs/raggr/raggr-frontend/node_modules <scratchpad>/wt-<ticket-id>/raggr-frontend/node_modules # only IMM-05
|
||||
herdr pane run <pane> "claude --model sonnet \"Implement the plan in <plan path>. Read it fully first. Your worktree is \$PWD.\""
|
||||
```
|
||||
Open one tab per wave, split into one pane per ticket (`--no-focus`, `--cwd`
|
||||
set to the worktree). Keep the ticket→pane mapping in `<scratchpad>/panes.md`.
|
||||
4. **Watch.** Run a Monitor loop over `herdr agent get <pane>` that reports when a
|
||||
pane becomes `idle|done|blocked`. Review each pane as soon as it finishes;
|
||||
don't wait for the whole wave.
|
||||
5. **Review twice per ticket.**
|
||||
- Read the worker's final report from its transcript (newest
|
||||
`~/.claude/projects/<cwd-slug>/*.jsonl`). Pane scrollback is too short to rely on.
|
||||
- Read the diff yourself: `git -C <wt> diff origin/main`, plus new files. Check it
|
||||
against the ticket's acceptance criteria and the design. Look at correctness,
|
||||
edge cases, security (the write guards in §8), anything changed silently,
|
||||
scope creep, and whether each test **fails without the change** (spot-check by
|
||||
reverting the fix).
|
||||
- Run the suite yourself in the worktree; a worker saying "tests pass" is not enough.
|
||||
- Send feedback as a numbered list with `file:line` and the fix you expect:
|
||||
`herdr pane run <pane> "<feedback>"`. Confirm the pane flips to `working`.
|
||||
- **Round two:** confirm every round-one item was actually fixed. If round two
|
||||
still finds a real bug, the conductor fixes it directly. There is no round three.
|
||||
6. **Ship, one PR per ticket.** In the worktree: re-run tests and lint; `git fetch`;
|
||||
rebase onto `origin/main` if it moved and re-test. Then **confirm nothing main gained
|
||||
was reverted** (for files main changed since your base, diff and check main's
|
||||
changes are still there). Commit with a message that explains *why*, plus the
|
||||
session's attribution lines. Then `git push -u origin <branch>` and
|
||||
`tea pr create -t "<title>" -d "<body>"`. The body covers the problem, the change,
|
||||
what review caught, and test numbers. Merge in ticket order when the user has
|
||||
asked for merging (`tea pr merge --style merge <n>`, after CI is green). If a
|
||||
merge is blocked, list the PRs and their order for the user and don't work around it.
|
||||
7. **Clean up.** Exit the workers, `herdr pane close` / `herdr tab close`,
|
||||
`git worktree remove` each worktree, and delete merged local branches.
|
||||
8. **Report.** Give a table of ticket → PR → state, what each review round
|
||||
caught, and anything left undone.
|
||||
|
||||
### Ownership of this file (sheepdog rule)
|
||||
|
||||
Several panes run at once, and each opens its own PR, so this file would be a
|
||||
merge-conflict magnet if everyone edited it. One pane owns it:
|
||||
|
||||
- **The conductor is the sheepdog, and the only writer of `tickets.md`.**
|
||||
Workers never edit it, and ticket PRs never touch it. A worker's diff that touches
|
||||
this file is sent back in review.
|
||||
- The conductor records status **once per wave**, after that wave's PRs merge.
|
||||
It makes one small commit on `docs/immich-mcp-tickets-wave-<n>` (tick checkboxes,
|
||||
add the PR numbers to the order table, file follow-ups as IMM-09+) and merges that
|
||||
PR before starting the next wave. No two branches edit the file at the same time,
|
||||
so there's nothing to race on.
|
||||
- Workers who find something ticket-worthy (a bug outside their scope, a design gap)
|
||||
put it under "open doubts" in their report. The conductor decides whether it becomes a ticket.
|
||||
- Live notes during a wave (pane mapping, review state) go in `<scratchpad>/panes.md`,
|
||||
not here.
|
||||
|
||||
### Worker ground rules (paste into every plan)
|
||||
|
||||
- Work **only** inside your worktree (your cwd). Don't touch other worktrees or
|
||||
`~/Programs/raggr`. No `git stash`, commits, pushes or PRs; the conductor ships.
|
||||
- Read `CLAUDE.md` and `docs/immich-mcp/design.md` first. Match the surrounding
|
||||
style: comment density, naming, and async everywhere.
|
||||
- Change only the files your ticket lists. If you think another file needs to
|
||||
change, say so in your report instead of editing it.
|
||||
- **Never edit `docs/immich-mcp/tickets.md`.** The conductor owns it (sheepdog rule).
|
||||
- Run `.venv/bin/pytest tests/unit -q` and `uvx ruff==0.8.2 check .`; both must
|
||||
pass. For IMM-05 also run `cd raggr-frontend && yarn tsc --noEmit`.
|
||||
- Never hand-write DB migrations (none of these tickets need one). If you think
|
||||
one is needed, stop and report.
|
||||
- New env vars go in **three places**: `.env.example`, the `x-raggr-common`
|
||||
environment block in `docker-compose.yml`, and the conductor's note to update `.env`.
|
||||
- Don't call a real Immich server from tests. Use `httpx.MockTransport` or stubs.
|
||||
- When you're done, reply with: files changed, what you did, the test and lint output
|
||||
(counts), and any open doubts. Feedback will arrive in this pane. Address it and
|
||||
reply in the same format.
|
||||
|
||||
### Repo conventions the conductor enforces
|
||||
|
||||
- Gitea with `tea` (not `gh`). PR flow: push, then `tea pr create`. "Kermit" means commit.
|
||||
- One logical change per branch/PR. Branch names follow the tickets below.
|
||||
- CI runs `ruff check .`, `pytest tests/unit`, `python -m scripts.evaluate_context`,
|
||||
and the frontend typecheck and build. All must be green before merging.
|
||||
- Never commit `.DS_Store`, `chroma_db/`, `chromadb/`, `.planning/`.
|
||||
|
||||
---
|
||||
|
||||
## 2. Ticket order
|
||||
|
||||
```
|
||||
IMM-01 (conductor: make keys, check Maine/partner facts)
|
||||
│ (must finish before IMM-04 merges)
|
||||
▼
|
||||
┌───────── Wave 1 ─────────┐
|
||||
│ IMM-02 server foundation │──┐
|
||||
│ IMM-03 summarize (pure) │──┤
|
||||
└──────────────────────────┘ │
|
||||
▼ ▼
|
||||
┌───────── Wave 2 ─────────────────────┐
|
||||
│ IMM-04 read tools (needs 02 + 03) │
|
||||
│ IMM-05 raggr wiring (needs 02) │
|
||||
└──────────────────────────────────────┘
|
||||
▼
|
||||
Wave 3: IMM-06 write tools (needs 04)
|
||||
▼
|
||||
Wave 4: IMM-07 docs + Claude Code (needs 05 + 06)
|
||||
▼
|
||||
Wave 5: IMM-08 live verification + rollout (conductor; needs everything deployed)
|
||||
```
|
||||
|
||||
| Order | Ticket | Owner | Depends on | Runs alongside | Merge order |
|
||||
|---|---|---|---|---|---|
|
||||
| 0 | IMM-01 Create scoped API keys, check Maine/partner facts (version already known: 3.3.1) | conductor | – | Wave 1 | (no PR; findings go in the wave-1 status commit) |
|
||||
| 1 | IMM-02 Server foundation: config, client, criteria, package skeleton | worker | – | IMM-03 | 1st |
|
||||
| 1 | IMM-03 Trip summaries: clustering, places, samples (pure functions) | worker | – | IMM-02 | 2nd (rebase on IMM-02) |
|
||||
| 2 | IMM-04 Phase 1 read tools in the MCP server | worker | 02, 03 | IMM-05 | 3rd |
|
||||
| 2 | IMM-05 Wire the server into raggr (admin agent, routing, UI, config) | worker | 02 | IMM-04 | 4th |
|
||||
| 3 | IMM-06 Phase 2 write tools behind `IMMICH_ENABLE_WRITES` | worker | 04 | – | 5th |
|
||||
| 4 | IMM-07 Usage docs, Claude Code registration, design follow-ups | worker | 05, 06 | – | 6th |
|
||||
| 5 | IMM-08 Live verification and rollout | conductor | all merged + deployed | – | – |
|
||||
|
||||
Why this order:
|
||||
|
||||
- **IMM-02 and IMM-03 don't share files.** IMM-03 works on plain dicts and never
|
||||
imports the client. IMM-03 must **not** create `immich_mcp/__init__.py`; IMM-02 owns
|
||||
it. Until then the package imports fine as a namespace package. Merge IMM-02 first,
|
||||
then rebase IMM-03.
|
||||
- **IMM-04 and IMM-05 don't share files.** IMM-04 only touches `immich_mcp/server.py` and
|
||||
its test. IMM-05 only touches raggr. Tool names are fixed by the design (§7), so
|
||||
IMM-05 can allowlist them before they exist. Its tests use stubs, as
|
||||
`test_homebox_mcp.py` does.
|
||||
- **IMM-06 waits for IMM-04** because both edit `immich_mcp/server.py`.
|
||||
- **IMM-07 comes last among the code tickets** so the docs describe what was merged,
|
||||
not what was planned.
|
||||
- **Phase 1 can deploy after IMM-05** with `IMMICH_ENABLE_WRITES` unset. Scenarios A,
|
||||
D, and the read halves of B and C work from then on.
|
||||
|
||||
---
|
||||
|
||||
## 3. Tickets
|
||||
|
||||
### [ ] IMM-01: Verify production Immich and create scoped API keys
|
||||
|
||||
- **Owner:** conductor (needs the Immich UI and an API key; no worker, no PR)
|
||||
- **Depends on:** nothing · **Blocks:** IMM-04 merge, IMM-08
|
||||
- **Why:** The API changed shape in v3.2.0 (§11), and the design has an open
|
||||
question (§13.2) that only the real server can answer.
|
||||
|
||||
**Already known (checked 2026-10-13):**
|
||||
|
||||
- `GET https://immich.ryanjchen.com/api/server/version` →
|
||||
`{"major":3,"minor":3,"patch":1}`. **Immich 3.3.1**, so the `filter` API style
|
||||
(nested `filter` plus `cursor` paging) is available and is the **default**. The legacy
|
||||
flat fields still work on 3.3.x, so keep both code paths and their golden tests.
|
||||
Production doesn't need `IMMICH_API_STYLE` set.
|
||||
- **Public URL:** `IMMICH_PUBLIC_URL=https://immich.ryanjchen.com`. Links in tool
|
||||
output use this. `IMMICH_URL` can stay on the LAN/compose address
|
||||
(`http://192.168.1.5:2283`) for the server's own traffic. If that address isn't
|
||||
reachable from the raggr container, use the public URL for both.
|
||||
|
||||
**Steps**
|
||||
|
||||
1. ~~Get the server version~~: done above (3.3.1). Re-run
|
||||
`curl -s https://immich.ryanjchen.com/api/server/version` right before IMM-08
|
||||
in case Immich was upgraded in between; a 4.x major means re-checking §11.
|
||||
2. In Immich → Account Settings → API Keys, create two keys for **Ryan's user**:
|
||||
- `simba-immich-read` with exactly `album.read`, `asset.read`,
|
||||
`asset.statistics`, `person.read`.
|
||||
- `simba-immich-write`: the same four plus `album.create` and `albumAsset.create`.
|
||||
Do **not** grant `all`, any `*.delete`, `album.update`, `album.share`,
|
||||
`asset.update` or `asset.share`.
|
||||
Store both in the password manager. Production `.env` starts with the **read**
|
||||
key (Phase 1).
|
||||
3. Using the read key, check the facts the design depends on (curl or httpie):
|
||||
- `GET /api/search/suggestions?type=state` contains `"Maine"` (confirms the
|
||||
reverse-geocoding spelling).
|
||||
- `POST /api/search/statistics` with `{"filter":{"state":{"eq":"Maine"}}}`
|
||||
returns a total, and the same total as legacy `{"state":"Maine"}`, which proves
|
||||
both paths agree on 3.3.1.
|
||||
- `GET /api/albums` returns albums including the existing Maine album. Record
|
||||
its name, `assetCount`, `startDate` and `endDate`; IMM-08 uses them.
|
||||
- **Partner photos (§13.2):** if a partner shares with Ryan with "show in
|
||||
timeline" on, check whether partner assets come back from
|
||||
`POST /api/search/metadata`. Write the answer down. Adding them to an album
|
||||
gets tested in IMM-08 with the write key.
|
||||
4. Check which `IMMICH_URL` the raggr container can reach:
|
||||
`docker compose exec raggr-<color> curl -s $IMMICH_URL/api/server/ping`.
|
||||
5. Put the results (partner answer, Maine album facts, chosen `IMMICH_URL`) in
|
||||
`<scratchpad>/imm-01-findings.md`, paste them into the IMM-04 and IMM-07
|
||||
plan files, and post them to the user. The conductor records them in the
|
||||
wave-1 status commit (sheepdog rule).
|
||||
|
||||
**Done when:** both keys exist with exactly the listed permissions; the Maine facts
|
||||
and the partner answer are recorded; the container-reachable `IMMICH_URL` is known.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-02: Server foundation (config, client, criteria, package skeleton)
|
||||
|
||||
- **Owner:** worker · **Branch:** `feat/immich-mcp-foundation` · **Base:** `origin/main`
|
||||
- **Depends on:** nothing · **Runs alongside:** IMM-03 · **Blocks:** IMM-03 merge, IMM-04, IMM-05
|
||||
- **PR title:** "Add the Immich MCP server package: config, API client and search criteria"
|
||||
- **Design:** §5, §6, §7.1, §8.5, §10, §11
|
||||
|
||||
**Goal.** Build everything the tools need except the tools themselves. When this
|
||||
ticket is done, `python -m immich_mcp` starts a FastMCP stdio server called
|
||||
`immich` with **no tools yet**, and the HTTP client and criteria model are fully
|
||||
tested.
|
||||
|
||||
**Files to create**
|
||||
|
||||
| File | Contents |
|
||||
|---|---|
|
||||
| `immich_mcp/__init__.py` | Module docstring only: what the package is, and that it runs as `python -m immich_mcp` over stdio. |
|
||||
| `immich_mcp/__main__.py` | `from immich_mcp.server import main` then `main()`. |
|
||||
| `immich_mcp/config.py` | Frozen `ImmichConfig` dataclass and `load_config(env=os.environ)`. |
|
||||
| `immich_mcp/client.py` | `ImmichClient` and `ImmichError`. |
|
||||
| `immich_mcp/criteria.py` | `PhotoCriteria`, `CriteriaError`, `local_day_bounds`, `to_search_body`, `to_statistics_body`, `resolve_place`. |
|
||||
| `immich_mcp/server.py` | `mcp = FastMCP("immich")`, `main()` that calls `mcp.run()` (stdio). No tools. |
|
||||
| `tests/unit/test_immich_config.py` | Config tests. |
|
||||
| `tests/unit/test_immich_client.py` | Client tests. |
|
||||
| `tests/unit/test_immich_criteria.py` | Criteria and body tests. |
|
||||
|
||||
**Files to modify**
|
||||
|
||||
- `Dockerfile`: add `COPY immich_mcp ./immich_mcp` after `COPY utils ./utils`
|
||||
(line 53). Without it the module is missing from the image; the Dockerfile copies
|
||||
directories explicitly.
|
||||
|
||||
**Do not change:** anything under `utils/`, `blueprints/` or `app.py`, or any existing test.
|
||||
|
||||
**Details**
|
||||
|
||||
1. **`config.py`**
|
||||
- Fields: `url` (from `IMMICH_URL`, trailing `/` stripped), `api_key`
|
||||
(`IMMICH_API_KEY`), `public_url` (`IMMICH_PUBLIC_URL`, default `url`),
|
||||
`timezone` (`ZoneInfo(IMMICH_TIMEZONE or TZ or "America/New_York")`),
|
||||
`writes_enabled` (`IMMICH_ENABLE_WRITES == "true"`), and `api_style`
|
||||
(`IMMICH_API_STYLE` in `{"legacy", "filter"}`, or `None` meaning auto-detect).
|
||||
- Missing `url` or `api_key` raises `ValueError` with a message that names the
|
||||
variable but **never prints the key**.
|
||||
- Constants live here: `MAX_RESULT_CHARS = 2500`, `MAX_SCAN = 5000`,
|
||||
`MAX_WRITE = 2000`, `PAGE_SIZE = 1000`, `WRITE_CHUNK = 500`.
|
||||
2. **`client.py`**
|
||||
- `ImmichClient(config, transport: httpx.AsyncBaseTransport | None = None)`
|
||||
wraps one `httpx.AsyncClient(base_url=f"{url}/api", headers={"x-api-key": key,
|
||||
"Accept": "application/json"}, timeout=30)`. Tests inject the transport. It is
|
||||
an async context manager.
|
||||
- `ALLOWED_ENDPOINTS: tuple[tuple[str, re.Pattern], ...]`, exactly:
|
||||
`GET /server/version`, `GET /albums`, `GET /albums/{uuid}`,
|
||||
`GET /search/suggestions`, `GET /search/person`, `POST /search/metadata`,
|
||||
`POST /search/smart`, `POST /search/statistics`, `POST /albums`,
|
||||
`PUT /albums/{uuid}/assets`. A private `_request(method, path, **kw)` raises
|
||||
`PermissionError` for anything else **before** sending. All public methods go
|
||||
through it.
|
||||
- Public methods (all `async`, returning parsed JSON or small dataclasses):
|
||||
`server_version() -> tuple[int, int, int]`;
|
||||
`list_albums(owned_only=False) -> list[dict]` (with no filter it returns owned
|
||||
and shared albums; `owned_only=True` sends `isOwned=true`);
|
||||
`get_album(album_id)`; `suggestions(kind, **filters) -> list[str]`;
|
||||
`search_person(name) -> list[dict]`;
|
||||
`search_metadata(body) -> dict` (one page); `search_smart(body) -> dict`;
|
||||
`statistics(body) -> int` (returns `total`);
|
||||
`create_album(name, description, asset_ids) -> dict`;
|
||||
`add_assets(album_id, asset_ids) -> list[dict]` (raw `BulkIdResponseDto` list).
|
||||
- `iter_metadata(body, *, limit) -> AsyncIterator[dict]` pages until `limit`
|
||||
assets or there are no more pages. It must support **both** paging styles: legacy
|
||||
(`page` numbers, `assets.nextPage`) and filter (`cursor`, `assets.nextCursor`).
|
||||
Choose based on whether the body has a `"filter"` key.
|
||||
- Errors: any non-2xx becomes `ImmichError(status, message)`, with `message`
|
||||
taken from Immich's JSON `message` field when present (truncated to 200 chars).
|
||||
A connection error becomes `ImmichError(0, "Immich is unreachable")`. Never put
|
||||
the API key in an exception or log line.
|
||||
- There are no `delete_*`, `update_*`, `remove_*` or `share_*` methods, and no
|
||||
generic `request()` method is public.
|
||||
3. **`criteria.py`**
|
||||
- `PhotoCriteria` (pydantic v2 `BaseModel`, `extra="forbid"`), fields exactly as
|
||||
in §7.1: `taken_after: date | None`, `taken_before: date | None`, `state`,
|
||||
`city`, `country`, `person: str | None`, `media: Literal["photo","video","all"] = "all"`,
|
||||
`favorites_only: bool = False`.
|
||||
- Validators: `taken_before >= taken_after` when both are set. Strip blank strings to
|
||||
`None`. `is_empty()` is true when no filter field is set (media `"all"` and
|
||||
favorites `False` count as unset).
|
||||
- `local_day_bounds(c, tz) -> (gte_utc | None, lt_utc | None)`:
|
||||
`taken_after` → local midnight → UTC. `taken_before` → **next day's** local
|
||||
midnight → UTC (exclusive). Return ISO strings with `Z`. Test across the DST
|
||||
changes on 2026-03-08 and 2026-11-01 in `America/New_York`.
|
||||
- `to_search_body(c, *, style, tz, person_ids=None, album_ids=None,
|
||||
exclude_album_id=None, size=PAGE_SIZE, with_exif=True) -> dict`:
|
||||
- `legacy`: `takenAfter`, `takenBefore` (use the exclusive bound minus 1 ms, because
|
||||
legacy `takenBefore` is inclusive), `state`, `city`, `country`, `personIds`,
|
||||
`albumIds`, `type` (`IMAGE`/`VIDEO`, omitted for `all`), `isFavorite`, `size`,
|
||||
`withExif`. Set only what's given; never send `null`. `exclude_album_id`
|
||||
is **not** supported in legacy mode: raise `ValueError` and let callers diff locally.
|
||||
- `filter`: `{"filter": {"takenAt": {"gte", "lt"}, "state": {"eq"}, "city": {"eq"},
|
||||
"country": {"eq"}, "personIds": {"all": [...]}, "albumIds": {"any": [...]},
|
||||
..."type": {"eq"}, "isFavorite": {"eq": true}}, "size", "withExif"}`.
|
||||
With `exclude_album_id`, use `"albumIds": {"none": [id]}`, and combine it
|
||||
correctly with `album_ids` (raise if both are given).
|
||||
- `to_statistics_body(...)`: same mapping without `size` or `withExif`.
|
||||
- `async resolve_place(client, kind, value) -> str`: case-insensitive exact match
|
||||
against `client.suggestions(kind)` returns Immich's spelling. If nothing matches,
|
||||
raise `CriteriaError("No <kind> called 'X'. Closest: A, B, C")` using
|
||||
`difflib.get_close_matches(value, options, n=3, cutoff=0.5)`.
|
||||
4. **`server.py`**: only the `FastMCP("immich")` instance, `main()`, and a module
|
||||
docstring that points at the design. IMM-04 adds the tools.
|
||||
|
||||
**Tests (each must fail without the code it covers)**
|
||||
|
||||
- Config: missing URL; missing key, with the message not containing the key value;
|
||||
`public_url` falls back to `url`; time zone falls back from `IMMICH_TIMEZONE` to `TZ`
|
||||
to the default; writes flag accepts only `"true"`; bad `IMMICH_API_STYLE` raises.
|
||||
- Client: the `x-api-key` header is sent; `DELETE` and `PATCH` raise
|
||||
`PermissionError` and the transport is **never called**; `/trash/empty` and
|
||||
`/assets` raise; `iter_metadata` stops at `limit` mid-page; it follows `nextPage`
|
||||
(legacy) and `nextCursor` (filter); a 400 with `{"message": "x"}` becomes
|
||||
`ImmichError(400, "x")`; a connect error becomes status 0; the key never appears
|
||||
in `str(error)`.
|
||||
- Criteria: golden-body tests for **both** styles for (a) a state only, (b) a date
|
||||
range only, (c) everything set; inclusive `taken_before` round-trips; DST bounds;
|
||||
empty criteria → `is_empty()`; reversed dates raise; `exclude_album_id` in legacy
|
||||
raises; `resolve_place` returns the canonical spelling, and gives three closest
|
||||
matches on a miss.
|
||||
- Smoke: `python -c "import immich_mcp.server"` imports, and `mcp.name == "immich"`.
|
||||
|
||||
**Acceptance**
|
||||
|
||||
- `.venv/bin/pytest tests/unit -q` and `uvx ruff==0.8.2 check .` are green.
|
||||
- `IMMICH_URL=http://x IMMICH_API_KEY=y .venv/bin/python -m immich_mcp` starts and
|
||||
waits on stdin (Ctrl-C to exit). With either var missing it exits non-zero with a
|
||||
clear message.
|
||||
- `grep -rn "delete\|DELETE\|patch\|PATCH" immich_mcp/` matches only the allowlist
|
||||
test helpers or comments.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-03: Trip summaries (clustering, places, samples)
|
||||
|
||||
- **Owner:** worker · **Branch:** `feat/immich-mcp-summaries` · **Base:** `origin/main`
|
||||
- **Depends on:** nothing · **Runs alongside:** IMM-02 · **Merge:** after IMM-02, rebased
|
||||
- **PR title:** "Summarize Immich search results into trip date clusters"
|
||||
- **Design:** §7.2 (output fields), §7.4 (clustering), §7 (size cap)
|
||||
|
||||
**Goal.** Pure, synchronous functions that turn a list of Immich asset dicts
|
||||
(shaped like `AssetResponseDto` with `exifInfo`) into the compact summary that
|
||||
`immich_search_photos` and `immich_album_gaps` return. They do no I/O and don't import
|
||||
`client.py` or `criteria.py`.
|
||||
|
||||
**Files to create**
|
||||
|
||||
- `immich_mcp/summarize.py`
|
||||
- `tests/unit/test_immich_summarize.py`
|
||||
- `tests/fixtures/immich/assets_maine_trip.json`: about 60 hand-built assets (see the tests below).
|
||||
|
||||
**Do not create** `immich_mcp/__init__.py`; IMM-02 owns it. Without it the package
|
||||
imports as a namespace package, which is fine for tests.
|
||||
|
||||
**Functions**
|
||||
|
||||
1. `asset_day(asset, tz) -> date | None`: the date part of `asset["localDateTime"]`
|
||||
parsed **as-is**, with no time-zone conversion (it is wall-clock time; §7.4). If
|
||||
that's missing, convert `fileCreatedAt` to `tz` and take the date. `None` if both are missing.
|
||||
2. `asset_place(asset) -> tuple[str | None, str | None]`: `(city, state)` from
|
||||
`exifInfo`, with empty strings treated as `None`.
|
||||
3. `cluster_by_day(assets, tz, gap_days=2, keep=8) -> list[dict]`: sort the distinct
|
||||
days, and start a new cluster when `(day - prev_day).days > gap_days`. Each cluster is
|
||||
`{"start","end","count","top_place"}`, where `top_place` is the most common
|
||||
`"City, State"` (or just the state, or `null`). Sort by `count` descending, then
|
||||
`start` descending; keep `keep`.
|
||||
4. `top_places(assets, n=5) -> list[{"place","count"}]`, plus a `no_location` count
|
||||
(returned as a tuple).
|
||||
5. `media_counts(assets) -> {"photo": n, "video": m}` (`type` is `IMAGE`/`VIDEO`).
|
||||
6. `pick_samples(assets, n, public_url, tz) -> list[dict]`: `n` evenly spaced assets
|
||||
after sorting by day, each `{"date","place","file","url"}` with
|
||||
`url = f"{public_url}/photos/{id}"`. `n=0` → `[]`.
|
||||
7. `summarize(assets, *, total, tz, public_url, samples=5, truncated=False) -> dict`
|
||||
with the keys `total, scanned, truncated, first, last, clusters, top_places,
|
||||
no_location, by_media, samples`. `criteria` is **not** included here; the tool adds it.
|
||||
8. `fit(result: dict, max_chars=2500) -> dict`: if `json.dumps(result,
|
||||
separators=(",",":"), ensure_ascii=False)` is over `max_chars`, trim lists in this
|
||||
order until it fits: `samples` (one at a time down to 0), `top_places` (down to 1),
|
||||
`clusters` (down to 1). Then set `"trimmed": true`. Never trim scalars. Return a
|
||||
new dict and don't mutate the input.
|
||||
|
||||
**Tests**
|
||||
|
||||
- The fixture has a trip Aug 2–9, 2026 (with **no photos on Aug 5**, to show a quiet
|
||||
day doesn't split the trip), a single day in 2023, and one asset with neither
|
||||
`localDateTime` nor `exifInfo`. Then `cluster_by_day` returns exactly two clusters,
|
||||
with the trip first: `start=2026-08-02`, `end=2026-08-09`.
|
||||
- `gap_days=2` boundary: days 1, 4 → two clusters; days 1, 3 → one.
|
||||
- `localDateTime` `"2026-08-09T23:30:00.000Z"` stays Aug 9 under
|
||||
`tz=America/Los_Angeles` and `tz=Asia/Tokyo` (no conversion).
|
||||
- The `fileCreatedAt` fallback converts: `2026-08-10T02:00:00Z` with New York → Aug 9.
|
||||
- `pick_samples` spreads samples evenly (first and last included when `n >= 2`) and
|
||||
builds correct URLs.
|
||||
- `fit` on a large synthetic result is at most 2500 chars, sets `trimmed`, trims samples
|
||||
before places before clusters, and leaves its input unchanged.
|
||||
- Empty input → `total` passes through, `first`/`last` are `null`, and lists are empty.
|
||||
|
||||
**Acceptance:** tests and ruff green; `summarize.py` imports only the standard library.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-04: Phase 1 read tools in the MCP server
|
||||
|
||||
- **Owner:** worker · **Branch:** `feat/immich-mcp-read-tools` · **Base:** `origin/main`
|
||||
**after IMM-02 and IMM-03 are merged**
|
||||
- **Depends on:** IMM-02, IMM-03 (production is 3.3.1, so `filter` is the style exercised live; both styles must still be tested)
|
||||
- **Runs alongside:** IMM-05 · **Blocks:** IMM-06
|
||||
- **PR title:** "Add read-only Immich tools: find albums, search photos, smart search, album gaps"
|
||||
- **Design:** §3 (scenarios A, B-read, C-read, D), §7, §11
|
||||
|
||||
**Goal.** Register the four Phase 1 tools on the FastMCP server, with exactly the
|
||||
names, parameters and outputs in §7.2. The server must be stateless: every call
|
||||
builds its own `ImmichClient`, because raggr starts a new process per call (§5).
|
||||
|
||||
**Files to modify:** `immich_mcp/server.py`.
|
||||
**Files to create:** `immich_mcp/tools_read.py` (the tool bodies, kept out of `server.py`
|
||||
so IMM-06 can add `tools_write.py` without conflicts) and `tests/unit/test_immich_server.py`.
|
||||
**Do not change:** `client.py`, `criteria.py` and `summarize.py`, except for bug
|
||||
fixes, which must be listed in the report.
|
||||
|
||||
**Details**
|
||||
|
||||
1. **A shared call context** `async with call_context() as ctx:` loads config,
|
||||
opens the client, and works out `ctx.style`: `config.api_style` if set, otherwise
|
||||
`filter` when `server_version() >= (3, 2, 0)` and `legacy` below that. It catches
|
||||
`ImmichError`, `CriteriaError` and `ValueError` and turns them into
|
||||
`{"error": "<sentence>"}`. Tools never raise to the MCP layer.
|
||||
2. **Tool names are registered without a prefix**: `find_albums`,
|
||||
`search_photos`, `smart_search`, `album_gaps` (and in IMM-06 `create_album`,
|
||||
`add_to_album`). raggr's `select_tools` adds `immich_`, as it does for Homebox,
|
||||
so Simba sees the `immich_*` names from design §7. Claude Code shows them as
|
||||
`mcp__immich__find_albums`. Descriptions that mention another tool use the
|
||||
raggr name (`immich_add_to_album`).
|
||||
3. **The criteria parameters are flat** on every tool that takes them (§7.1): the
|
||||
tool function signatures declare `taken_after: str | None = None` (ISO date
|
||||
string), `taken_before`, `state`, `city`, `country`, `person`, `media`,
|
||||
`favorites_only` directly. Build `PhotoCriteria` inside. **Test that the
|
||||
generated input schema has no `$defs` or `$ref`.**
|
||||
4. **`normalize(ctx, criteria) -> (criteria, person_ids)`** resolves `state`, `city`
|
||||
and `country` through `resolve_place`, and `person` through `search_person`:
|
||||
exactly one case-insensitive name match is required, otherwise an error listing
|
||||
up to 5 candidate names.
|
||||
5. **`find_albums(query="", limit=10)`**: `list_albums()`, then rank: exact
|
||||
(case-insensitive) match first, then substring matches, then
|
||||
`difflib.SequenceMatcher` ratio ≥ 0.6, ties broken by `updatedAt` descending.
|
||||
An empty query returns the newest `limit` by `updatedAt`. Output is
|
||||
`{"albums": [{"id","name","asset_count","start","end","shared","owned","url"}]}`.
|
||||
`owned` comes from a second call, `GET /albums?isOwned=true` (add an
|
||||
`owned_only: bool = False` parameter to `client.list_albums`): an album is owned
|
||||
if its ID is in that set. This needs no extra API-key permission. `start`/`end`
|
||||
are converted to local dates in `config.timezone`. `limit` is clamped to 1–25.
|
||||
6. **`search_photos(<criteria>, samples=5)`**: reject empty criteria with
|
||||
`{"error": "Give at least a date range, place, or person."}`. Then
|
||||
`statistics()` for the total, `iter_metadata(limit=MAX_SCAN)`, and
|
||||
`fit(summarize(...) | {"criteria": <normalized flat criteria dict, dates as ISO strings>})`.
|
||||
7. **`smart_search(query, <criteria>, limit=10)`**: requires `query`; the
|
||||
criteria are optional. `search_smart` with the body from `to_search_body` plus
|
||||
`query`, and `size=limit` (clamped 1–25). Output is
|
||||
`{"results": [{"date","place","file","url"}]}` in rank order, passed through `fit`.
|
||||
8. **`album_gaps(album_id, pad_days=1)`**: `get_album`. With no
|
||||
`startDate` → error. The window is `[start - pad, end + pad]` in local dates.
|
||||
Then a **shared helper** `resolve_missing(ctx, album_id, criteria, limit)` that
|
||||
IMM-06 will reuse:
|
||||
- `filter` style: one search with `exclude_album_id=album_id`.
|
||||
- `legacy` style: collect the album's asset IDs (`iter_metadata` with
|
||||
`album_ids=[id]`, no limit up to `MAX_WRITE * 5`), then the window's assets,
|
||||
minus that set.
|
||||
- Returns `(missing_assets, truncated)`.
|
||||
Output: `summarize(missing)` plus `"album": {"id","name","url"}`,
|
||||
`"missing_count"`, and `"criteria"` (the window).
|
||||
9. **Tool descriptions** are one or two sentences each and at most 300 chars, written for the
|
||||
model, for example: *"Find photo albums by (partial) name. Call this first when the user
|
||||
mentions a trip or event."* The total schema size across all four tools, measured as
|
||||
`sum(len(json.dumps(t.inputSchema)) + len(t.description))`, must be **at most 4000
|
||||
chars** (about 1000 tokens), which leaves room for the two Phase 2 tools within the
|
||||
~1500-token budget in §2.6.
|
||||
|
||||
**Tests (`test_immich_server.py`)**
|
||||
|
||||
Use `mcp.shared.memory.create_connected_server_and_client_session(server._mcp_server)`
|
||||
and inject a fake Immich through a module-level hook such as
|
||||
`server.TRANSPORT_FACTORY = lambda: httpx.MockTransport(handler)`. Cover:
|
||||
|
||||
- `list_tools()` returns exactly the four names, with no `$ref` in any schema, within
|
||||
the schema budget.
|
||||
- Scenario A: albums "Maine 2026" and "Acadia / Maine" are both found for "maine",
|
||||
with the exact match ranked first. An empty query returns the newest first.
|
||||
- Scenario B (read): state "maine" resolves to "Maine"; the summary has two clusters;
|
||||
`criteria.state == "Maine"`; the total comes from statistics, not from the scan length.
|
||||
- A misspelled state ("Main") → an error with "Closest: Maine".
|
||||
- An ambiguous person → an error listing the candidates.
|
||||
- Empty criteria → the error, and **no HTTP call** is made beyond what's needed for setup.
|
||||
- Scenario C (read), in **both** styles: the missing photos are the window minus the
|
||||
album, and `missing_count` matches.
|
||||
- Scenario D: smart search passes `query`, clamps `limit` to 25, and keeps rank order.
|
||||
- Version detection: 3.1.9 → legacy bodies; 3.2.0 → filter bodies; the
|
||||
`IMMICH_API_STYLE` override skips the version call.
|
||||
- An Immich 500 or a connection error → `{"error": ...}`, not an exception.
|
||||
- Every tool result is at most 2500 chars, even with a 5000-asset fixture.
|
||||
|
||||
**Acceptance:** tests and ruff green. Manually, `npx @modelcontextprotocol/inspector
|
||||
.venv/bin/python -m immich_mcp` (with real env, optional) lists four tools. Report the
|
||||
measured per-call process start time: `time` around one `call_tool` through
|
||||
`langchain_mcp_adapters` with a fake URL is fine.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-05: Wire the server into raggr (admin agent, routing, UI, config)
|
||||
|
||||
- **Owner:** worker · **Branch:** `feat/immich-mcp-raggr` · **Base:** `origin/main`
|
||||
after IMM-02 merged
|
||||
- **Depends on:** IMM-02 · **Runs alongside:** IMM-04 · **Merge:** after IMM-04
|
||||
- **PR title:** "Load the Immich MCP tools into Simba's admin agent"
|
||||
- **Design:** §6 (raggr-side list), §8.2, §9, §10
|
||||
|
||||
**Goal.** raggr starts `python -m immich_mcp` the way it starts Homebox and
|
||||
BookStack: admin-only, prefixed `immich_`, allowlisted, routed, labelled and
|
||||
configured. Nothing user-visible happens unless `IMMICH_URL` and `IMMICH_API_KEY` are set.
|
||||
|
||||
**Files and exact changes**
|
||||
|
||||
1. `utils/mcp_tools.py`: `load_stdio_tools` (line 83) gains keyword-only
|
||||
`args: list[str] | None = None` and `cwd: str | None = None`. Pass
|
||||
`"args": args or []` (line 100) and, only when given, `"cwd": cwd`. Homebox and
|
||||
BookStack callers stay unchanged. Add a test that the connection dict gets `args`/`cwd`.
|
||||
2. **New** `utils/immich_mcp.py`, mirroring `utils/homebox_mcp.py`:
|
||||
- `IMMICH_READ_TOOLS = ("find_albums", "search_photos", "smart_search",
|
||||
"album_gaps")` and `IMMICH_WRITE_TOOLS = ("create_album", "add_to_album")`.
|
||||
The server registers these **unprefixed** names (IMM-04 detail 2) and
|
||||
`select_tools` adds the `immich_` prefix, like Homebox.
|
||||
- `immich_allowlist() -> tuple`: read tools, plus write tools when
|
||||
`IMMICH_ENABLE_WRITES == "true"`. This keeps `select_tools` from warning about
|
||||
"missing tools" when writes are off.
|
||||
- `load_immich_tools()`: returns `[]` with an info log if `IMMICH_URL` or
|
||||
`IMMICH_API_KEY` is unset. Otherwise
|
||||
`load_stdio_tools(server="immich", command=os.getenv("IMMICH_MCP_COMMAND", sys.executable),
|
||||
args=[] if os.getenv("IMMICH_MCP_COMMAND") else ["-m", "immich_mcp"],
|
||||
cwd=str(Path(__file__).resolve().parents[1]), env={IMMICH_URL, IMMICH_API_KEY,
|
||||
IMMICH_PUBLIC_URL, IMMICH_TIMEZONE, IMMICH_ENABLE_WRITES, IMMICH_API_STYLE — only
|
||||
the ones that are set}, allowlist=immich_allowlist(), prefix="immich", label="Immich")`.
|
||||
3. `app.py` line 134: `admin_tools = await load_homebox_tools() + await
|
||||
load_bookstack_tools() + await load_immich_tools()`, plus the import next to
|
||||
lines 45–46.
|
||||
4. `blueprints/conversation/tool_routing.py`:
|
||||
- `GROUP_TERMS["photos"] = "immich photo photos picture pictures pic pics album
|
||||
albums trip trips vacation"`.
|
||||
- `tool_group`: add `("immich_", "photos")` to the prefix list.
|
||||
- `ToolRouter.__init__` (line 87): change `startswith(("homebox", "bookstack"))` to
|
||||
`startswith(("homebox", "bookstack", "immich"))`.
|
||||
- Check that adding "album" or "trip" doesn't change any existing routing test.
|
||||
("page"/"pages" belong to `knowledge`; there's no overlap.)
|
||||
5. `blueprints/conversation/tool_labels.py` line 50: add
|
||||
`("immich_", "Looking through photos…")` to `TOOL_PREFIX_LABELS`.
|
||||
6. `raggr-frontend/src/components/ActivityTimeline.tsx`: import `Images` from
|
||||
`lucide-react` (line 23 import block), and add `["immich", Images]` to
|
||||
`TOOL_ICONS` (after `bookstack`).
|
||||
7. `blueprints/conversation/prompts.py` line 39: insert the §9 sentence right after
|
||||
the BookStack sentence, word for word. Keep `test_prompts.py` green; if it pins exact text,
|
||||
update the expectation in the same PR.
|
||||
8. `blueprints/integrations/__init__.py` line 92 admin list: add
|
||||
`("Immich", ("IMMICH_URL", "IMMICH_API_KEY"))` after BookStack.
|
||||
9. `config/validation.py`: add `"IMMICH_URL"` and `"IMMICH_PUBLIC_URL"` to the
|
||||
URL list (line 168), `"IMMICH_ENABLE_WRITES"` to the true/false list (line 159),
|
||||
and validate `IMMICH_API_STYLE ∈ {legacy, filter}` when set.
|
||||
10. `.env.example` (lines 66–70, the existing Immich block): keep `IMMICH_URL` and
|
||||
`IMMICH_API_KEY`, and add commented `IMMICH_PUBLIC_URL=`, `IMMICH_TIMEZONE=`,
|
||||
`IMMICH_ENABLE_WRITES=false`, `IMMICH_API_STYLE=`, with a one-line comment
|
||||
on the scoped-key permissions (§8.6). Keep `SEARCH_QUERY`/`DOWNLOAD_DIR` for the indexer.
|
||||
11. `docker-compose.yml` `x-raggr-common` environment block (near
|
||||
`HOMEBOX_URL` at line 91): `IMMICH_URL`, `IMMICH_API_KEY`, `IMMICH_PUBLIC_URL`,
|
||||
`IMMICH_TIMEZONE`, `IMMICH_ENABLE_WRITES`, `IMMICH_API_STYLE`, written as
|
||||
`- NAME=${NAME}` (or `${NAME:-}` to match neighbours).
|
||||
|
||||
**Tests**
|
||||
|
||||
- **New** `tests/unit/test_immich_mcp.py`, modelled on `test_homebox_mcp.py`: allowlist
|
||||
filtering and prefixing; write tools absent unless the flag is on; the admin gate
|
||||
refuses non-admins without calling the server; missing env → `[]` with no subprocess;
|
||||
the env passed to `load_stdio_tools` contains only `IMMICH_*` keys (patch it and inspect);
|
||||
default command is `sys.executable` with args `-m immich_mcp` and `cwd` is the repo root;
|
||||
`IMMICH_MCP_COMMAND` override → `args == []`.
|
||||
- `test_tool_routing.py`: "find my maine trip photos" routes to `photos` for admins;
|
||||
`immich_*` tools are hidden from non-admins in both static and dynamic modes.
|
||||
- `test_configuration.py`: a bad `IMMICH_URL` and a bad `IMMICH_ENABLE_WRITES` are
|
||||
rejected; an empty Immich config is fine (optional integration).
|
||||
- `tool_labels`: `tool_label("immich_find_albums") == "Looking through photos…"`.
|
||||
- Integration status: admins see an `Immich` row, non-admins don't.
|
||||
|
||||
**Acceptance:** backend tests, ruff, `yarn tsc --noEmit` and `yarn build` all green.
|
||||
With `IMMICH_*` unset the app boots exactly as before (log line "Immich MCP not
|
||||
configured").
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-06: Phase 2 write tools behind `IMMICH_ENABLE_WRITES`
|
||||
|
||||
- **Owner:** worker · **Branch:** `feat/immich-mcp-write-tools` · **Base:** `origin/main`
|
||||
after IMM-04 merged (IMM-05 merged too, ideally)
|
||||
- **Depends on:** IMM-04 · **Blocks:** IMM-07
|
||||
- **PR title:** "Let Simba create Immich albums and add photos after confirmation"
|
||||
- **Design:** §3 (B and C end to end), §7.3, §8 (every layer)
|
||||
|
||||
**Goal.** Add `create_album` and `add_to_album` (prefixed to `immich_*` by raggr)
|
||||
with **every** write guard in §8. Nothing in this ticket can delete or remove anything.
|
||||
|
||||
**Files to create:** `immich_mcp/tools_write.py`, `tests/unit/test_immich_write_tools.py`.
|
||||
**Files to modify:** `immich_mcp/server.py` (register the write tools **only if**
|
||||
`load_config().writes_enabled` is true at startup), and `immich_mcp/tools_read.py` only if
|
||||
`resolve_missing` needs a small signature change (say so in the report).
|
||||
|
||||
**Details**
|
||||
|
||||
1. **`create_album(name, expected_count, <criteria>, description="")`**
|
||||
1. Strip `name`; if it's empty, error.
|
||||
2. `list_albums()`. If any album name matches case-insensitively → `{"error":
|
||||
"An album named 'X' already exists (N photos). Use immich_add_to_album to add to it.",
|
||||
"album": {id, name, url}}`.
|
||||
3. Normalize the criteria (reuse `normalize`). Empty criteria → the error from IMM-04.
|
||||
4. `total = statistics(...)`. If `total > MAX_WRITE` → error asking the user to narrow
|
||||
the range. Then collect **all** IDs with `iter_metadata(limit=MAX_WRITE)`. Request
|
||||
only IDs where the API allows; otherwise take `id` from each item.
|
||||
5. If `len(ids) != expected_count` → `{"error": "The search now matches N photos,
|
||||
not the M you confirmed. Show the user the new count and ask again.",
|
||||
"actual_count": N}`, and make **zero** write calls.
|
||||
6. `create_album(name, description, ids[:500])`, then `add_assets` for the rest in
|
||||
chunks of 500.
|
||||
7. Return `{"album": {"id","name","url"}, "added": n}`. If chunk N fails partway,
|
||||
return what was added so far plus `"error"`. Don't retry; the album exists and
|
||||
the user can run `add_to_album`.
|
||||
2. **`add_to_album(album_id, expected_new_count, <criteria>)`**
|
||||
1. Normalize the criteria, which can't be empty. `get_album` must succeed. If the
|
||||
album isn't in `list_albums(owned_only=True)`, error before resolving anything
|
||||
(Immich might reject it anyway, but this message is clearer).
|
||||
2. `missing, truncated = resolve_missing(ctx, album_id, criteria, limit=MAX_WRITE)`.
|
||||
If `truncated` → the too-many error.
|
||||
3. If `len(missing) != expected_new_count` → the count-mismatch error, with no writes.
|
||||
4. `add_assets` in chunks of 500. Tally `success`, `error == "duplicate"`
|
||||
(→ `already_present`), and other errors (→ `failed`, keeping at most 5
|
||||
`{"id","reason"}` items).
|
||||
5. Return `{"album": {...}, "added", "already_present", "failed"}`.
|
||||
3. **Descriptions** must say plainly: *"Only call after the user has said yes to the
|
||||
exact count you showed them. Pass that count as expected_count."* Keep both within
|
||||
the remaining schema budget (§2.6; all six tools together are at most about 6000 chars).
|
||||
4. **Registration.** With the flag off, `list_tools()` has exactly the four read tools.
|
||||
With it on, six.
|
||||
|
||||
**Tests (`test_immich_write_tools.py`, in-memory session + MockTransport)**
|
||||
|
||||
- Flag off → no write tools listed, and calling one → MCP "unknown tool" error.
|
||||
- Scenario B end to end: search → create with the matching count → one `POST /albums`
|
||||
with 455 IDs split 500/… (use 1,200 IDs to check chunking: one POST + two PUTs).
|
||||
- Count mismatch → no `POST`/`PUT` (assert on the recorded requests).
|
||||
- Duplicate name (case-insensitive) → error, and **no** search or write calls after `list_albums`.
|
||||
- `total > MAX_WRITE` → error, no writes.
|
||||
- Scenario C end to end in both API styles: gaps → add with `missing_count` →
|
||||
the PUT body has exactly the missing IDs.
|
||||
- Results with `duplicate` and `no_permission` are tallied correctly.
|
||||
- A partial failure on the second PUT → `added` reflects the first chunk, plus `error`.
|
||||
- The recorded requests across the **whole test module** contain no `DELETE` or `PATCH`
|
||||
(a module-scoped fixture asserts this at teardown).
|
||||
|
||||
**Acceptance:** tests and ruff green. The report shows a sample request log from the
|
||||
scenario B test. The conductor checks every §8 layer against the code during review and
|
||||
lists them in the PR body.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-07: Usage docs, Claude Code registration, design follow-ups
|
||||
|
||||
- **Owner:** worker · **Branch:** `docs/immich-mcp-usage` · **Base:** `origin/main`
|
||||
after IMM-05 and IMM-06 are merged
|
||||
- **Depends on:** IMM-05, IMM-06 (and the IMM-01 findings)
|
||||
- **PR title:** "Document the Immich MCP tools and how to use them from Claude Code"
|
||||
|
||||
**Files**
|
||||
|
||||
- **New** `docs/immich-mcp/usage.md`:
|
||||
1. What Simba can do with photos, using the four scenarios from design §3 as example
|
||||
prompts.
|
||||
2. Setup: create the scoped key (exact permission lists from IMM-01), the env vars
|
||||
table (copied from design §10, with the real defaults as merged), and how to turn
|
||||
writes on.
|
||||
3. **Claude Code:** the `claude mcp add immich …` command from design §10, using the
|
||||
real paths. Say that Claude Code keeps one long-lived process (unlike raggr's
|
||||
per-call processes); nothing in the server depends on either.
|
||||
4. Troubleshooting: "Immich MCP not configured" log; `{"error": "Immich is unreachable"}`;
|
||||
count-mismatch errors (expected: the library changed); a 403 on writes means the key
|
||||
lacks `album.create`/`albumAsset.create`; `IMMICH_API_STYLE` override.
|
||||
5. The safety model in five bullets (design §8, short form).
|
||||
- `docs/immich-mcp/design.md`: set the status to "implemented (Phase 1 + 2)"; in §13,
|
||||
fill in the answers from IMM-01 (version, partner photos); note any departures from
|
||||
the design that IMM-02–06 made (taken from their PR bodies). At minimum, add to §7
|
||||
that the server registers unprefixed names and raggr adds `immich_` (IMM-04 detail 2),
|
||||
and add to §7.2 that `owned` comes from `GET /albums?isOwned=true`.
|
||||
- `mkdocs.yml`: add `Immich MCP (usage): immich-mcp/usage.md` next to the design entry.
|
||||
- `CLAUDE.md`: under `utils/`, add `immich_mcp.py` (the loader); under the architecture
|
||||
list, add a short **`immich_mcp/`** entry (in-repo stdio MCP server for Immich albums
|
||||
and photo search; admin-only in raggr; also usable from Claude Code; see
|
||||
`docs/immich-mcp/`). Add `IMMICH_*` to the household-integrations env line.
|
||||
- Do **not** edit `docs/immich-mcp/tickets.md`. The conductor ticks it (sheepdog rule).
|
||||
|
||||
**Acceptance:** `mkdocs build --strict` passes, if mkdocs is available locally (otherwise
|
||||
check the links by hand). Every command in `usage.md` was actually run once by the worker
|
||||
or the conductor, except the ones that need production.
|
||||
|
||||
---
|
||||
|
||||
### [ ] IMM-08: Live verification and rollout
|
||||
|
||||
- **Owner:** conductor (needs production, the user's Immich, and judgment calls)
|
||||
- **Depends on:** IMM-02 to IMM-07 merged; `make redeploy` done
|
||||
- **Design:** §3, §14
|
||||
|
||||
**Phase 1 (read key, `IMMICH_ENABLE_WRITES` unset)**
|
||||
|
||||
1. Put `IMMICH_URL`, the read `IMMICH_API_KEY`, `IMMICH_PUBLIC_URL` and, if IMM-01
|
||||
decided it, `IMMICH_API_STYLE` in production `.env`. Run `make redeploy`. `make logs`
|
||||
should show "Immich MCP: loaded 4 tools".
|
||||
2. In Simba's web chat **as an admin**, run scenarios A, B (stop at "Want an album?"),
|
||||
C (stop at "Add them?") and D from design §3. For each one, record the tool calls
|
||||
from the activity timeline, the answer, the links (they must open in a browser), and
|
||||
rough latency.
|
||||
3. As a **non-admin** user, ask "find my maine photos". The answer must not use
|
||||
`immich_*` tools (check the timeline), and the integration status page has no Immich row.
|
||||
4. If a result is wrong (bad clusters, a missed album, a link to the wrong host), file a
|
||||
follow-up ticket here (IMM-09+) with the evidence. Don't patch production by hand.
|
||||
|
||||
**Phase 2 (write key)**
|
||||
|
||||
5. Swap in the write key and set `IMMICH_ENABLE_WRITES=true`, then `make redeploy`.
|
||||
Expect "loaded 6 tools".
|
||||
6. Scenario B end to end with the album name **"ZZ Simba test – delete me"** over a
|
||||
small, known date range (one day). Check the album in Immich has exactly the confirmed
|
||||
count.
|
||||
7. Count-mismatch drill: get a count, then upload one photo from that day on a phone,
|
||||
then confirm with the old count. Simba must refuse and re-show the new count.
|
||||
8. Scenario C end to end on the test album: remove a few photos from it in the Immich UI,
|
||||
ask what's missing, and confirm the add.
|
||||
9. Partner photos (design §13.2): if partner sharing exists, include a partner photo's
|
||||
day and record whether the add works or comes back in `failed`.
|
||||
10. Delete the test album **by hand in the Immich UI**. Simba can't.
|
||||
|
||||
**Claude Code**
|
||||
|
||||
11. Register the server on the laptop with the `usage.md` command. In a fresh Claude Code
|
||||
session, ask "do I have a Maine album?" and check it answers with the link.
|
||||
|
||||
**Done when:** every step has recorded evidence (paste a short table into the IMM-08 PR
|
||||
or a comment on the design PR); any follow-ups are filed as IMM-09+ in this file; the
|
||||
production `.env` matches the final intended state (the write key only if the user wants
|
||||
writes left on; ask them).
|
||||
Reference in New Issue
Block a user