docs: update CLAUDE.md + migration plan to reflect shipped Postgres design
CLAUDE.md updates: - new src/second_brain/embeddings/ entry in the project layout - setup section now lists the postgres-superuser bootstrap (CREATE SCHEMA AUTHORIZATION lovebug) and the alembic step - env var table now covers SECOND_BRAIN_DATABASE_URL, OLLAMA_URL, EMBEDDING_MODEL, and the pool sizing knobs - conventions section gained five new gotchas (the lovebug CREATE gap, public.embeddings ownership, best-effort embedding, etc.) postgres-migration-planning.md flipped from "planning questions" to a "what shipped" runbook — locked decisions table, operator setup steps, known gotchas, and the next-iteration backlog.
This commit is contained in:
parent
a2d46a7c6e
commit
6c1445c8ed
41
CLAUDE.md
41
CLAUDE.md
@ -22,18 +22,36 @@ pending → pulled → transcribed → analyzed → accepted → published
|
|||||||
src/second_brain/
|
src/second_brain/
|
||||||
├── adapters/ # Pull adapters (youtube.py, article.py)
|
├── adapters/ # Pull adapters (youtube.py, article.py)
|
||||||
├── context/ # Context assembler
|
├── context/ # Context assembler
|
||||||
|
├── embeddings/ # Chunk + nomic-embed-text + upsert into public.embeddings
|
||||||
├── extractor/ # LLM extraction engine + Pydantic schema
|
├── extractor/ # LLM extraction engine + Pydantic schema
|
||||||
├── llm/ # claude_cli.py — thin subprocess wrapper around `claude -p`
|
├── llm/ # claude_cli.py — thin subprocess wrapper around `claude -p`
|
||||||
├── compiler/ # Wiki compiler (reads vault, writes pages, git commits)
|
├── compiler/ # Wiki compiler (reads vault, writes pages, git commits)
|
||||||
├── scheduler/ # Rate-limit smoother (call-count for CLI, token-budget for API)
|
├── scheduler/ # Rate-limit smoother (call-count for CLI, token-budget for API)
|
||||||
└── web/ # FastAPI + Jinja2 + HTMX UI
|
└── web/ # FastAPI + Jinja2 + HTMX UI
|
||||||
|
|
||||||
|
alembic/ # Schema migrations for the second_brain Postgres schema
|
||||||
```
|
```
|
||||||
|
|
||||||
## Setup
|
## Setup
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
cp config/settings.toml.example config/settings.toml
|
cp config/settings.toml.example config/settings.toml
|
||||||
# Edit settings.toml with your paths
|
# Edit settings.toml — at minimum, point the paths and set SECOND_BRAIN_DATABASE_URL.
|
||||||
|
|
||||||
|
# Connection string format. Containerised (default):
|
||||||
|
# postgresql+psycopg://lovebug:<password>@homelab-postgres:5432/petalbrain
|
||||||
|
# Host-side dev runs:
|
||||||
|
# postgresql+psycopg://lovebug:<password>@127.0.0.1:5433/petalbrain
|
||||||
|
export SECOND_BRAIN_DATABASE_URL=...
|
||||||
|
|
||||||
|
# One-time, before first migration: bootstrap the schema as postgres
|
||||||
|
# superuser. lovebug doesn't have CREATE on petalbrain, so this can't be
|
||||||
|
# done from the app side — same pattern as wiki/trellis/ob1.
|
||||||
|
# CREATE SCHEMA second_brain AUTHORIZATION lovebug;
|
||||||
|
# GRANT ALL PRIVILEGES ON SCHEMA second_brain TO lovebug;
|
||||||
|
|
||||||
|
uv sync
|
||||||
|
uv run alembic upgrade head
|
||||||
uv run second-brain serve
|
uv run second-brain serve
|
||||||
```
|
```
|
||||||
|
|
||||||
@ -58,6 +76,10 @@ uv run second-brain serve
|
|||||||
|
|
||||||
- `ANTHROPIC_API_KEY` — only required when `extractor.backend = "api"`. The default `cli` backend uses your Max OAuth and does not need it.
|
- `ANTHROPIC_API_KEY` — only required when `extractor.backend = "api"`. The default `cli` backend uses your Max OAuth and does not need it.
|
||||||
- `SECOND_BRAIN_CONFIG` — optional override for the settings file path.
|
- `SECOND_BRAIN_CONFIG` — optional override for the settings file path.
|
||||||
|
- `SECOND_BRAIN_DATABASE_URL` — Postgres connection string. Falls back to `HERBYLAB_DATABASE_URL` if unset. Required at runtime; both `second_brain.database` and `second_brain.embeddings` read it. Use the SQLAlchemy `postgresql+psycopg://` form; the embeddings module strips the `+psycopg` prefix internally for raw psycopg.
|
||||||
|
- `OLLAMA_URL` — overrides `[embeddings].ollama_url`. Default `http://ollama:11434` (homelab docker network); for host-side runs use `http://127.0.0.1:11434`.
|
||||||
|
- `EMBEDDING_MODEL` — overrides `[embeddings].model`. Default `nomic-embed-text` (768-d).
|
||||||
|
- `SECOND_BRAIN_DB_POOL_MIN` / `SECOND_BRAIN_DB_POOL_MAX` — SQLAlchemy pool sizing knobs (defaults 1 / 9 overflow).
|
||||||
|
|
||||||
## Domains
|
## Domains
|
||||||
|
|
||||||
@ -68,24 +90,31 @@ uv run second-brain serve
|
|||||||
|
|
||||||
## Key files
|
## Key files
|
||||||
|
|
||||||
- `config/settings.toml` — vault path, DB path, scheduler window, `[extractor]` backend config
|
- `config/settings.toml` — vault path, `[database]`, `[extractor]`, `[embeddings]`, `[scheduler]`
|
||||||
- `prompts/<domain>.md` — per-domain extraction prompt templates
|
- `prompts/<domain>.md` — per-domain extraction prompt templates
|
||||||
- `src/second_brain/extractor/schema.py` — `LLMExtraction` (LLM-only fields, drives the `--json-schema` contract) + `ExtractionResult` (LLMExtraction + SourceMeta filled from the DB)
|
- `src/second_brain/extractor/schema.py` — `LLMExtraction` (LLM-only fields, drives the `--json-schema` contract) + `ExtractionResult` (LLMExtraction + SourceMeta filled from the DB)
|
||||||
- `src/second_brain/llm/claude_cli.py` — CLI subprocess wrapper
|
- `src/second_brain/llm/claude_cli.py` — CLI subprocess wrapper
|
||||||
- `src/second_brain/models.py` — SQLAlchemy models + `utcnow()` helper (replaces deprecated `datetime.utcnow`)
|
- `src/second_brain/models.py` — SQLAlchemy models (all tables live in the `second_brain` schema) + `utcnow()` helper
|
||||||
- `postgres-migration-planning.md` — open questions blocking the SQLite → Postgres+pgvector migration
|
- `src/second_brain/database.py` — Postgres engine + URL resolution
|
||||||
|
- `src/second_brain/embeddings/__init__.py` — chunk → Ollama → upsert into `public.embeddings`; best-effort with graceful degradation
|
||||||
|
- `alembic/` + `alembic.ini` — schema migrations for the `second_brain` schema (Alembic does **not** manage `public.embeddings`)
|
||||||
|
- `postgres-migration-planning.md` — original planning doc; superseded by this file once the migration shipped
|
||||||
|
|
||||||
## Conventions / gotchas
|
## Conventions / gotchas
|
||||||
|
|
||||||
- **Naive UTC timestamps everywhere.** Use `from second_brain.models import utcnow` rather than `datetime.utcnow()` (deprecated in 3.12) or `datetime.now()` (local time).
|
- **Naive UTC timestamps everywhere.** Use `from second_brain.models import utcnow` rather than `datetime.utcnow()` (deprecated in 3.12) or `datetime.now()` (local time). Postgres columns are `timestamp without time zone` — do not switch to `timestamptz`.
|
||||||
|
- **PK style.** Int autoincrement (`SERIAL`) — locked for compatibility with the existing schema shape. Do not switch to `BIGINT GENERATED ALWAYS AS IDENTITY` or UUID.
|
||||||
- **Re-query inside the session.** SQLAlchemy ORM attribute access after the session closes raises `DetachedInstanceError`. The `process` command and the web accept/reject endpoints both collect IDs first, then re-query inside the per-iteration / per-request session block, and pre-compute any dict views needed for templates *inside* the `with db.session()` block.
|
- **Re-query inside the session.** SQLAlchemy ORM attribute access after the session closes raises `DetachedInstanceError`. The `process` command and the web accept/reject endpoints both collect IDs first, then re-query inside the per-iteration / per-request session block, and pre-compute any dict views needed for templates *inside* the `with db.session()` block.
|
||||||
- **Starlette `TemplateResponse` signature.** Starlette ≥ 0.36 requires `templates.TemplateResponse(request, name, context)`. The old `(name, context)` form raises `TypeError: unhashable type: 'dict'`. Do not put `"request": request` into the context dict — it's auto-injected now.
|
- **Starlette `TemplateResponse` signature.** Starlette ≥ 0.36 requires `templates.TemplateResponse(request, name, context)`. The old `(name, context)` form raises `TypeError: unhashable type: 'dict'`. Do not put `"request": request` into the context dict — it's auto-injected now.
|
||||||
- **Vault git repo auto-init.** The wiki compiler calls `_ensure_git_repo()` on every run, which `git init`s the vault and creates an empty seed commit if no `.git` is present. `_git_commit()` also checks `git diff --cached --quiet` before committing to avoid empty commits.
|
- **Vault git repo auto-init.** The wiki compiler calls `_ensure_git_repo()` on every run, which `git init`s the vault and creates an empty seed commit if no `.git` is present. `_git_commit()` also checks `git diff --cached --quiet` before committing to avoid empty commits.
|
||||||
- **Scheduler throttling shape.** The CLI backend is throttled by `max_calls_per_hour` (Max subscription limits are call-shaped, not token-shaped). The API backend uses `max_tokens_per_hour`. Both share `min_gap_seconds`. The scheduler targets sources in `TRANSCRIBED` state — earlier code mistakenly filtered on `ANALYZED`, making it a no-op.
|
- **Scheduler throttling shape.** The CLI backend is throttled by `max_calls_per_hour` (Max subscription limits are call-shaped, not token-shaped). The API backend uses `max_tokens_per_hour`. Both share `min_gap_seconds`. The scheduler targets sources in `TRANSCRIBED` state — earlier code mistakenly filtered on `ANALYZED`, making it a no-op.
|
||||||
|
- **Alembic + lovebug privilege gap.** `lovebug` owns the `second_brain` schema but does **not** have `CREATE` on the `petalbrain` database. `alembic/env.py` therefore guards `CREATE SCHEMA` behind a `pg_namespace` lookup, and the v1 migration intentionally drops the `IF NOT EXISTS` form (a bare `CREATE SCHEMA IF NOT EXISTS` raises permission denied even when the schema is already present). The schema is bootstrapped once, out-of-band, by `postgres` superuser.
|
||||||
|
- **public.embeddings is shared, not ours.** Alembic only manages the `second_brain` schema. The vector table itself is owned by `postgres` and shared across vault-mcp / ob1 / second-brain — we just INSERT into it. Rows are keyed by `(source_schema='second_brain', source_table='extractions', source_id, embedding_model='nomic-embed-text')`.
|
||||||
|
- **Embedding is best-effort.** The pipeline writes the relational `extractions` row first and `sess.flush()`es; only then calls `embed_extraction()`. Any failure (no DB URL, no Ollama, embedding 500) logs at WARNING and returns None — the relational row and file vault stay canonical.
|
||||||
|
- **File-based wiki, not petalbrain.wiki.** The wiki compiler still writes markdown into the configured `vault_path` and commits via git. We do **not** dual-write into `petalbrain.wiki` — that's vault-mcp's territory and intentionally out of scope.
|
||||||
|
|
||||||
## In-flight work
|
## In-flight work
|
||||||
|
|
||||||
- **Postgres + pgvector migration.** SQLite is the current backing store. Travis wants to move to his existing Postgres + pgvector instance and use the shared `/projects/shared/python/embedding_chunking` module for vector indexing. Planning questions are enumerated in `postgres-migration-planning.md` at the project root; no migration work has started yet.
|
|
||||||
- **Vault location.** Recommended `/opt/projects/second-brain-vault/` as the production location (peer to the project dir). Not yet confirmed.
|
- **Vault location.** Recommended `/opt/projects/second-brain-vault/` as the production location (peer to the project dir). Not yet confirmed.
|
||||||
|
|
||||||
## Validation
|
## Validation
|
||||||
|
|||||||
@ -1,78 +1,118 @@
|
|||||||
# Postgres + pgvector migration — planning questions
|
# Postgres + pgvector migration — implementation notes
|
||||||
|
|
||||||
Status: awaiting input from Travis before scoping the migration.
|
Status: **shipped** (2026-05-24). Originally a planning checklist; rewritten
|
||||||
Trigger: user pivoted away from SQLite — wants to use the existing Postgres + pgvector instance, plus the shared `/projects/shared/python/embedding_chunking` module.
|
to capture the implemented design and the decisions that landed. Kept in the
|
||||||
|
repo so the next iteration has the "why" without grepping commits.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 1. Connection / secrets
|
## What shipped
|
||||||
|
|
||||||
- [ ] Host, port, database name, user
|
- Backing store switched from SQLite to the existing **petalbrain** Postgres
|
||||||
- [ ] Where does the connection string live? Options:
|
instance (pgvector/pg17), connecting over the homelab docker network at
|
||||||
- env var (e.g. `SECOND_BRAIN_DB_URL`)
|
`homelab-postgres:5432`.
|
||||||
- `config/settings.toml` (would need a `.local.toml` override so the password doesn't get committed)
|
- Relational tables (`sources`, `extractions`, `wiki_pages`) live in a new
|
||||||
- `.pgpass`
|
`second_brain` schema owned by the **`lovebug`** role — no dedicated service
|
||||||
- other
|
role.
|
||||||
- [ ] Is the Postgres instance on `herbydev`, the NAS, or elsewhere? Tailscale-only or LAN-reachable?
|
- Embeddings reuse the **shared `public.embeddings` table** (HNSW vector index
|
||||||
- [ ] Driver preference:
|
already in place). Rows are keyed by
|
||||||
- `psycopg` v3 sync (default suggestion — pipeline + scheduler are sync today)
|
`(source_schema='second_brain', source_table='extractions', source_id, embedding_model='nomic-embed-text')`.
|
||||||
- `asyncpg` (only worth it if we also rewrite the web layer to async)
|
For this round we embed **extraction summaries only** (not transcript
|
||||||
|
chunks, not key_points / claims).
|
||||||
## 2. Schema layout
|
- Embedding pipeline mirrors `vault_mcp.core.embeddings`: shared
|
||||||
|
`embedding_chunking` for 512/64-token chunking → Ollama
|
||||||
- [ ] Same Postgres **instance** as `vault-mcp`'s `petalbrain.wiki`? Yes / No
|
`nomic-embed-text` (768-d, `num_ctx=8192`) → delete-before-insert upsert
|
||||||
- [ ] If yes — same **database** with a dedicated schema (e.g. `second_brain.sources`, `second_brain.extractions`)? Or its own database entirely?
|
with vector text literals (`%s::vector`).
|
||||||
- [ ] Naming conventions to match:
|
- **Graceful degradation** is the rule: missing DB URL, Ollama down, or any
|
||||||
- snake_case tables — assumed yes
|
DB error logs a warning and returns `None`. The relational row and the
|
||||||
- `created_at` / `updated_at` as `timestamptz` (vs current naive UTC) — preference?
|
file-based vault stay canonical.
|
||||||
- PK type: `bigserial` / `bigint identity` / UUID?
|
- **Alembic** stands up the `second_brain` schema; `public.embeddings` is
|
||||||
- [ ] Should second-brain integrate with `vault-mcp` at all?
|
managed out-of-band and intentionally outside the migration tree.
|
||||||
- Option A: standalone — second-brain owns its tables, the wiki compiler still writes Obsidian markdown files
|
- File-based wiki compiler is unchanged. No dual-write to `petalbrain.wiki`.
|
||||||
- Option B: dual-write — published extractions land in `petalbrain.wiki` via `vault-mcp` so they show up in the unified wiki
|
- **No SQLite import** — we started clean on Postgres.
|
||||||
- Option C: replace the file-based compiler entirely with `vault-mcp` writes
|
|
||||||
|
|
||||||
## 3. Embeddings
|
|
||||||
|
|
||||||
- [ ] Public API of `/projects/shared/python/embedding_chunking`:
|
|
||||||
- function signatures
|
|
||||||
- what it returns (raw text chunks? chunks + vectors? chunker only, embeddings done separately?)
|
|
||||||
- embedding model + vector dimension (drives the `vector(N)` column type)
|
|
||||||
- any batching / rate-limit behavior we need to respect
|
|
||||||
- [ ] Where should embeddings attach?
|
|
||||||
- (a) on `Extraction` rows — semantic search over summaries / key_points / claims
|
|
||||||
- (b) on chunked transcripts — chunk-level retrieval for "find the part of the video that says X"
|
|
||||||
- (c) both (separate tables: `extraction_embeddings` + `transcript_chunks`)
|
|
||||||
- [ ] pgvector index strategy:
|
|
||||||
- HNSW — better recall, slower build, recommended default for personal-scale corpus
|
|
||||||
- IVFFlat — faster build, needs periodic `ANALYZE` + a meaningful `lists` value
|
|
||||||
- [ ] Distance metric: cosine (typical), L2, or inner product?
|
|
||||||
|
|
||||||
## 4. Migration tooling
|
|
||||||
|
|
||||||
- [ ] Schema is small and not yet in production. Cleanest path:
|
|
||||||
- SQLAlchemy `Base.metadata.create_all()` against Postgres
|
|
||||||
- one-off import script to move existing SQLite rows over (if any are worth keeping)
|
|
||||||
- [ ] Alternative: stand up Alembic now while the cost is low. Pays off the first time you `ALTER` a table in anger; costs ~1 session of setup.
|
|
||||||
- [ ] Travis's call — defaulting to `create_all` + import script unless you say otherwise.
|
|
||||||
|
|
||||||
## 5. Pooling
|
|
||||||
|
|
||||||
- [ ] Three consumers share the DB: CLI (`process`, `compile`), the FastAPI web server, the scheduler.
|
|
||||||
- [ ] Default plan: SQLAlchemy `QueuePool` with `pool_size=5, max_overflow=5`.
|
|
||||||
- [ ] Is PgBouncer already in front of this instance? If yes — switch to `NullPool` on our side and let PgBouncer pool.
|
|
||||||
|
|
||||||
## 6. Other open items (carryover from prior session)
|
|
||||||
|
|
||||||
- [ ] Vault location decision: I recommended `/opt/projects/second-brain-vault/` as a peer dir. Not yet confirmed.
|
|
||||||
- [ ] NAS mount: is it mounted on `herbydev`, and at what path? Relevant if media (downloaded videos, transcripts) should live there rather than on the herbydev SSD.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## What I will produce once these are answered
|
## Locked decisions (from the planning round)
|
||||||
|
|
||||||
1. Updated `src/second_brain/database.py` with a Postgres engine + URL resolution
|
| Question | Decision |
|
||||||
2. Schema migration: new tables for embeddings (and chunks if we go with option (c))
|
|---|---|
|
||||||
3. Wiring of `embedding_chunking` into the extractor (or into a post-extraction step in the scheduler)
|
| DB instance | Existing petalbrain (homelab-postgres) |
|
||||||
4. `config/settings.toml.example` updates with the new `[database]` and `[embeddings]` blocks
|
| Schema | New `second_brain` schema (owned by `lovebug`) |
|
||||||
5. README / CLAUDE.md updates noting the Postgres dependency
|
| Connection | Containerised: `homelab-postgres:5432` (NOT host `127.0.0.1:5433`) |
|
||||||
6. One-shot SQLite → Postgres import script (only if there are rows worth keeping)
|
| Pooling | `psycopg_pool.ConnectionPool` min=1/max=10 — no PgBouncer in front |
|
||||||
|
| Driver | psycopg v3, SQLAlchemy uses `postgresql+psycopg://` |
|
||||||
|
| PK style | Int autoincrement (`SERIAL`) — kept |
|
||||||
|
| Timestamps | Naive UTC `timestamp without time zone` — kept |
|
||||||
|
| Embedding scope | Extraction summaries only (Phase 1) |
|
||||||
|
| Embedding model | `nomic-embed-text` (768-d), Ollama via `OLLAMA_URL` |
|
||||||
|
| Migration tool | Alembic, second-brain owns its own tree |
|
||||||
|
| `public.embeddings` | NOT managed by our Alembic — shared with vault-mcp / ob1 |
|
||||||
|
| File-based wiki | Kept intact; no dual-write into petalbrain.wiki |
|
||||||
|
| Old SQLite data | Not imported — clean start |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Setup (operator runbook)
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# 1. One-time, as postgres superuser, bootstrap the schema. lovebug doesn't
|
||||||
|
# have CREATE on petalbrain, so this can't be done from the app side.
|
||||||
|
docker exec homelab-postgres psql -U postgres -d petalbrain -c \
|
||||||
|
"CREATE SCHEMA second_brain AUTHORIZATION lovebug;
|
||||||
|
GRANT ALL PRIVILEGES ON SCHEMA second_brain TO lovebug;"
|
||||||
|
|
||||||
|
# 2. Point the project at the DB (env wins over settings.toml).
|
||||||
|
export SECOND_BRAIN_DATABASE_URL=\
|
||||||
|
"postgresql+psycopg://lovebug:<pw>@homelab-postgres:5432/petalbrain"
|
||||||
|
|
||||||
|
# 3. Install deps and run the migration.
|
||||||
|
uv sync
|
||||||
|
uv run alembic upgrade head
|
||||||
|
```
|
||||||
|
|
||||||
|
The `lovebug` password lives in `/opt/backups/postgres-consolidation/credentials.env`
|
||||||
|
(0600, host-only).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Key files (post-migration)
|
||||||
|
|
||||||
|
- `src/second_brain/models.py` — schema-qualified ORM, `__table_args__` →
|
||||||
|
`second_brain`; native Postgres enum types are scoped to the schema too.
|
||||||
|
- `src/second_brain/database.py` — psycopg-v3 engine + URL resolution
|
||||||
|
(constructor arg → `SECOND_BRAIN_DATABASE_URL` → `HERBYLAB_DATABASE_URL`
|
||||||
|
→ `[database].url`).
|
||||||
|
- `src/second_brain/embeddings/__init__.py` — near-verbatim port of
|
||||||
|
`vault_mcp/core/embeddings.py` adapted to our key tuple.
|
||||||
|
- `alembic.ini` + `alembic/env.py` — schema-pinned Alembic; the env.py guards
|
||||||
|
`CREATE SCHEMA` behind `pg_namespace` because `lovebug` lacks `CREATE` on
|
||||||
|
the database.
|
||||||
|
- `alembic/versions/4a1e2f6c9d10_v1_second_brain_pipeline_tables.py` — v1
|
||||||
|
migration: enum types + `sources` + `extractions` + `wiki_pages`.
|
||||||
|
- `tests/test_smoke_embedding.py` — end-to-end smoke test exercising the full
|
||||||
|
Postgres + Ollama path. Auto-skips when either is unreachable.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Known gotchas
|
||||||
|
|
||||||
|
1. **`CREATE SCHEMA IF NOT EXISTS` errors as `lovebug`** even when the schema
|
||||||
|
exists. `pg_namespace` lookup in `alembic/env.py` is the workaround.
|
||||||
|
2. **`+psycopg` URL prefix.** SQLAlchemy needs it; raw psycopg doesn't.
|
||||||
|
`database.py` adds it if missing; the embeddings module strips it.
|
||||||
|
3. **Pool conflict.** `database.py` opens an SQLAlchemy pool; `embeddings/__init__.py`
|
||||||
|
opens an independent psycopg pool. Both share the same DSN; both are small
|
||||||
|
(min=1) so they don't strain the cluster.
|
||||||
|
4. **`anthropic-skills:zero-check` needs `pytest-cov`.** Added to the new
|
||||||
|
`[dependency-groups].dev` block so `uv sync` resolves it.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Next iterations (open, not blocked)
|
||||||
|
|
||||||
|
- Embed transcript chunks (or at least key_points / claims), not just summaries.
|
||||||
|
- Promote vector retrieval into the web UI (semantic search across extractions).
|
||||||
|
- Add a periodic backfill script that re-embeds extractions whose
|
||||||
|
`embedding_model` no longer matches the current default.
|
||||||
|
- Decide on the production vault path (`/opt/projects/second-brain-vault/`?).
|
||||||
|
|||||||
30
reviews/a-review-2026-05-24-2.md
Normal file
30
reviews/a-review-2026-05-24-2.md
Normal file
@ -0,0 +1,30 @@
|
|||||||
|
# a-review — 2026-05-24
|
||||||
|
|
||||||
|
**Model:** gemini
|
||||||
|
**Diff base:** `HEAD~1` (12 files changed, 208 insertions(+), 21 deletions(-))
|
||||||
|
**Session reference:** none
|
||||||
|
**Context:** CLAUDE.md + session notes + expanded diff
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
Based on the provided context, here is my code review.
|
||||||
|
|
||||||
|
### [INFO] Project Dependencies
|
||||||
|
|
||||||
|
* **File:** `pyproject.toml`
|
||||||
|
* **Finding:** The change introduces a `[dependency-groups].dev` section to manage development-specific dependencies like `pytest`, `pytest-cov`, and `ruff`.
|
||||||
|
* **Reasoning:** This is a positive change that follows standard Python project structure. It cleanly separates the project's runtime dependencies from the tools required for testing and linting, which improves dependency management and clarifies the project's setup for new developers.
|
||||||
|
|
||||||
|
### [INFO] Code Cleanup
|
||||||
|
|
||||||
|
* **Files:** `src/second_brain/adapters/article.py`, `src/second_brain/adapters/youtube.py`, `src/second_brain/compiler/wiki.py`, `src/second_brain/context/assembler.py`, `src/second_brain/extractor/engine.py`, `src/second_brain/extractor/schema.py`, `src/second_brain/main.py`, `src/second_brain/scheduler/runner.py`, `src/second_brain/web/app.py`
|
||||||
|
* **Finding:** Multiple unused imports (`Optional`, `datetime`, `json`, `Path`, etc.) were removed from several files.
|
||||||
|
* **Reasoning:** This is a beneficial code hygiene practice. Removing unused imports reduces clutter, makes the code easier to read, and helps linters and static analysis tools run more effectively. It has no impact on runtime behavior but improves maintainability.
|
||||||
|
|
||||||
|
### Summary
|
||||||
|
|
||||||
|
The submitted changes are of high quality. They consist of two main improvements:
|
||||||
|
1. Properly defining development dependencies in `pyproject.toml`.
|
||||||
|
2. Cleaning up unused imports across the codebase, likely as a result of running a linter like the newly added `ruff`.
|
||||||
|
|
||||||
|
I found no security vulnerabilities, bugs, or convention violations. The changes are safe and improve the project's overall maintainability and structure. The implementation aligns with the goal of setting up a better development and testing environment.
|
||||||
65
reviews/a-review-2026-05-24-3.md
Normal file
65
reviews/a-review-2026-05-24-3.md
Normal file
@ -0,0 +1,65 @@
|
|||||||
|
# a-review — 2026-05-24
|
||||||
|
|
||||||
|
**Model:** gemini
|
||||||
|
**Diff base:** `4696d7f` (24 files changed, 1496 insertions(+), 144 deletions(-))
|
||||||
|
**Session reference:** none
|
||||||
|
**Context:** CLAUDE.md + session notes + expanded diff
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
Excellent, the changes look solid overall. This is a substantial and well-executed migration from SQLite to Postgres, with the bonus of a new embedding feature. The code quality is high, particularly the database and transaction management logic.
|
||||||
|
|
||||||
|
I have a few findings, mostly related to documentation and test robustness.
|
||||||
|
|
||||||
|
### [WARNING] Documentation is now out of sync with the codebase
|
||||||
|
|
||||||
|
* **Location:** `CLAUDE.md`
|
||||||
|
* **Issue:** The project's main documentation file, `CLAUDE.md`, has not been updated to reflect these significant changes.
|
||||||
|
1. It still states the "Postgres + pgvector migration" is "In-flight work" and refers to `postgres-migration-planning.md`. This change appears to complete that migration.
|
||||||
|
2. The "Project layout" and "Key files" sections are missing the new `alembic/` directory for migrations and the new `src/second_brain/embeddings/` module.
|
||||||
|
3. The configuration sections do not mention the new `[database]` or `[embeddings]` tables in `config/settings.toml`.
|
||||||
|
* **Impact:** New developers (or future you!) will be confused by the discrepancy between the documentation and the actual state of the code. This makes maintenance harder and increases the risk of mistakes.
|
||||||
|
* **Recommendation:** After a large feature lands, it's crucial to update the project's core documentation. `CLAUDE.md` should be updated to reflect that the project now uses Postgres, describe the new embedding pipeline, and document the new configuration options. The `postgres-migration-planning.md` file should likely be archived or deleted.
|
||||||
|
|
||||||
|
### [INFO] Test fragility due to hardcoded string literal
|
||||||
|
|
||||||
|
* **Location:** `tests/test_smoke_embedding.py`, line 111
|
||||||
|
* **Issue:** The test query hardcodes the embedding model name: `AND embedding_model = 'nomic-embed-text'`. The application code correctly reads the model from the configuration. If the default model in `config/settings.toml` were to change in the future, this test would fail even if the application logic is correct.
|
||||||
|
* **Impact:** This creates a tight coupling between the test and a specific configuration value, making the test suite more brittle.
|
||||||
|
* **Recommendation:** The test should load the application `Config` and use the configured embedding model name in its verification query. This ensures the test is checking that the system works with *whatever* model is currently configured.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# tests/test_smoke_embedding.py
|
||||||
|
|
||||||
|
# at the top
|
||||||
|
from second_brain.config import load_config
|
||||||
|
|
||||||
|
# in the test function
|
||||||
|
config = load_config()
|
||||||
|
embedding_model = config.embeddings.get("model", "nomic-embed-text")
|
||||||
|
|
||||||
|
# ... later in the raw SQL query
|
||||||
|
cur.execute(
|
||||||
|
"""
|
||||||
|
SELECT COUNT(*) ...
|
||||||
|
WHERE ...
|
||||||
|
AND embedding_model = %s
|
||||||
|
""",
|
||||||
|
(extraction_id, embedding_model),
|
||||||
|
)
|
||||||
|
```
|
||||||
|
|
||||||
|
### [INFO] Unused imports removed
|
||||||
|
|
||||||
|
* **Location:** Multiple files, including `src/second_brain/adapters/article.py`, `src/second_brain/compiler/wiki.py`, `src/second_brain/extractor/schema.py`, etc.
|
||||||
|
* **Issue:** The diff removes several unused imports. While cleaning this up is good, the fact they were present suggests a linter (like `ruff`, which is in the dev dependencies) may not be running automatically as part of the development or pre-commit workflow.
|
||||||
|
* **Impact:** This is a minor code hygiene issue, but consistent linting helps catch small issues early and maintains code quality across the project.
|
||||||
|
* **Recommendation:** Consider setting up a pre-commit hook to run `ruff check --fix` to automate the removal of unused imports and fix other simple linting issues.
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Overall, this is a high-quality contribution. The migration to Postgres is thoughtful and robust, correctly using Alembic for schema management and a connection pool for performance. The implementation of the embedding pipeline is well-contained, idempotent, and gracefully handles potential failures. The addition of a comprehensive smoke test is a fantastic practice that significantly increases confidence in the new functionality.
|
||||||
|
|
||||||
|
The primary area for improvement is in process: the project's documentation needs to be treated as part of the code and updated in the same change that implements new features or makes significant architectural modifications.
|
||||||
|
|
||||||
|
Aside from the documentation gap, the code is clean, correct, and demonstrates a strong understanding of the technologies involved. Great work.
|
||||||
Loading…
Reference in New Issue
Block a user