# 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.