Small cleanups from platform-prep code review #31

New issue

Open

opened 2026-04-19 12:21:42 -06:00 by claude-code · 0 comments

claude-code commented

2026-04-19 12:21:42 -06:00

Collaborator

Umbrella issue for non-blocking follow-ups surfaced during review of the #26/#27 PR. None of these are correctness bugs; all are "land when convenient" polish.

1. Move `logger = ...` below all imports in `service.py`

src/quartermaster/service.py:10 declares logger = logging.getLogger("quartermaster.service") between the stdlib imports (lines 2–5) and the project imports (lines 12–19). Every other module that follows this pattern (notably month_service.py) declares the logger after all imports. One-line move for style consistency.

2. Comment in `routes_health.py` about router separation vs middleware

The spec and plan describe keeping /healthz on its own router so "future middleware" leaves it alone. That's not quite right: FastAPI middleware registered via app.add_middleware(...) runs on every request regardless of router. The router boundary only helps if future auth is applied as a router-scoped dependencies=[...] (which is fine — Quartermaster's auth is Traefik-side anyway, so the point is moot in practice).

Add a short comment near the router declaration in src/quartermaster/routes_health.py clarifying this, so a future maintainer adding app-level middleware doesn't assume /healthz is automatically exempt.

3. Richer `template_entry_updated` log extras

Today service.update_entry logs {event, entry_id}. For the Loki signal to be useful during "my Power bill amount looks wrong after editing" investigations, include the names of the fields that changed (not their values — values can contain PII-ish notes, and an audit trail belongs in a table, not in logs). Something like:

changed = [
    k for k, v in (("name", name), ("amount", amount)) if v is not None
] + (["notes"] if notes is not _NOTES_SENTINEL else [])
logger.info(
    "updated template entry",
    extra={
        "event": "template_entry_updated",
        "entry_id": entry.id,
        "fields": changed,
    },
)

Update the seed-event test to assert fields is present on the record.

Nits not tracked separately

Several smaller items surfaced in review but aren't worth their own bullet — fold into whichever PR lands items above if natural:

Consistency: only one of the five seed-event tests asserts levelname == "INFO". Either all should or none should.
OperationalError("boom", None, Exception("boom")) in tests/test_health.py is a legitimate but cosmetically odd constructor shape.
README LogQL code blocks lack a ```logql language tag.
year_month could be added to posting_added / posting_deleted extras for easier month-scoped queries in Loki.
README Run block could note --log-config is optional in dev for human-readable output.

Umbrella issue for non-blocking follow-ups surfaced during review of the #26/#27 PR. None of these are correctness bugs; all are "land when convenient" polish. ## 1. Move `logger = ...` below all imports in `service.py` `src/quartermaster/service.py:10` declares `logger = logging.getLogger("quartermaster.service")` *between* the stdlib imports (lines 2–5) and the project imports (lines 12–19). Every other module that follows this pattern (notably `month_service.py`) declares the logger after all imports. One-line move for style consistency. ## 2. Comment in `routes_health.py` about router separation vs middleware The spec and plan describe keeping `/healthz` on its own router so "future middleware" leaves it alone. That's not quite right: FastAPI middleware registered via `app.add_middleware(...)` runs on every request regardless of router. The router boundary only helps if future auth is applied as a router-scoped `dependencies=[...]` (which is fine — Quartermaster's auth is Traefik-side anyway, so the point is moot in practice). Add a short comment near the router declaration in `src/quartermaster/routes_health.py` clarifying this, so a future maintainer adding app-level middleware doesn't assume `/healthz` is automatically exempt. ## 3. Richer `template_entry_updated` log extras Today `service.update_entry` logs `{event, entry_id}`. For the Loki signal to be useful during "my Power bill amount looks wrong after editing" investigations, include the *names* of the fields that changed (not their values — values can contain PII-ish notes, and an audit trail belongs in a table, not in logs). Something like: ```python changed = [ k for k, v in (("name", name), ("amount", amount)) if v is not None ] + (["notes"] if notes is not _NOTES_SENTINEL else []) logger.info( "updated template entry", extra={ "event": "template_entry_updated", "entry_id": entry.id, "fields": changed, }, ) ``` Update the seed-event test to assert `fields` is present on the record. ## Nits not tracked separately Several smaller items surfaced in review but aren't worth their own bullet — fold into whichever PR lands items above if natural: - Consistency: only one of the five seed-event tests asserts `levelname == "INFO"`. Either all should or none should. - `OperationalError("boom", None, Exception("boom"))` in `tests/test_health.py` is a legitimate but cosmetically odd constructor shape. - README LogQL code blocks lack a ```logql language tag. - `year_month` could be added to `posting_added` / `posting_deleted` extras for easier month-scoped queries in Loki. - README Run block could note `--log-config` is optional in dev for human-readable output.

claude-code referenced this issue

2026-04-19 18:36:07 -06:00

docs: Session 3 — deploy pipeline live #36

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: archeious/quartermaster#31

No description provided.

Small cleanups from platform-prep code review #31

1. Move logger = ... below all imports in service.py

2. Comment in routes_health.py about router separation vs middleware

3. Richer template_entry_updated log extras

Nits not tracked separately

1. Move `logger = ...` below all imports in `service.py`

2. Comment in `routes_health.py` about router separation vs middleware

3. Richer `template_entry_updated` log extras