Skip to content

fix: close DB connections, honor path env vars, gate CI on tests#8

Closed
vidaks wants to merge 1 commit into
mainfrom
review-fixes-2026-06-16
Closed

fix: close DB connections, honor path env vars, gate CI on tests#8
vidaks wants to merge 1 commit into
mainfrom
review-fixes-2026-06-16

Conversation

@vidaks

@vidaks vidaks commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

A design/architecture/implementation review of Colophon surfaced a batch of small but real findings. This PR addresses them together — no behaviour change for existing deployments (every new knob defaults to the prior behaviour).

Findings fixed

  • SQLite connection leak. Store._conn() used the bare sqlite3 context manager, which commits but never closes the handle — an fd leak and a ResourceWarning storm in the long-running maintain process. _conn is now a @contextmanager that commits and closes. (Verified: the suite now passes under -W error::ResourceWarning.)
  • COLOPHON_DB / COLOPHON_REPORTS ignored. Both were documented (README, .env.example) but never read — the changelog DB and reports always used repo-relative paths. They now override those defaults; unset ⇒ unchanged. The five duplicated report-directory blocks in the CLI collapsed into one _reports_dir() helper.
  • CI never ran the tests. The workflow compiled and linted but skipped the 69-test, network-free suite, despite the docs stating it gates CI. Added the unittest discover step across Python 3.9–3.12.
  • Token-mint errors leaked as tracebacks. Grimmory.token() let a raw urllib error (a bad X-Edda-Proxy-Auth secret or an unreachable server) escape past the CLI's GrimmoryError/PreconditionError handler. It is now a GrimmoryError naming the endpoint and cause.
  • Dedup docs overclaim. The README/CHANGELOG described an automated "collapse duplicate records / move the file onto the keeper" feature that was never built. The audit only identifies duplicate groups and suggests a keeper; corrected to say so.
  • Dead constant. Removed the misleading MAX_PER_RUN from heal.py (the per-run cap lives in backfill.py; heal_book enforces none).

Verification

  • python -m compileall colophon — clean
  • python -W error::ResourceWarning -m unittest discover -s tests — 69 pass
  • ruff check . — clean
  • COLOPHON_DB / COLOPHON_REPORTS overrides exercised manually (DB + reports dir created at the overridden paths)

🤖 Generated with Claude Code

A design/architecture/implementation review surfaced a set of small but real
findings; this addresses them together.

- Store leaked SQLite connections: _conn() used the bare sqlite3 context
  manager, which commits but never closes the handle — an fd leak and a
  ResourceWarning storm in the long-running maintain process. _conn is now a
  contextmanager that commits AND closes.

- COLOPHON_DB and COLOPHON_REPORTS were documented (README, .env.example) but
  never read; the changelog DB and reports always used repo-relative paths.
  Both now override their defaults (unset => unchanged behaviour). The five
  duplicated report-directory blocks in the CLI collapsed into one helper.

- CI compiled and linted but never ran the 69-test network-free suite, despite
  the docs stating it gates CI. Added the unittest step across Python 3.9-3.12.

- Grimmory.token() let a raw urllib error (a bad X-Edda-Proxy-Auth secret or an
  unreachable server) escape past the CLI's GrimmoryError/PreconditionError
  handler as a traceback; it is now a GrimmoryError naming the endpoint.

- Corrected the dedup docs: the README/CHANGELOG described an automated
  "collapse duplicate records" feature that was never built. The audit only
  identifies duplicate groups and suggests a keeper.

- Removed a dead, misleading MAX_PER_RUN constant from heal.py (the per-run cap
  lives in backfill.py; heal_book enforces none).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@vidaks

vidaks commented Jun 16, 2026

Copy link
Copy Markdown
Owner Author

Landed on main as 232fe02 (squash). gh pr merge --squash hit a transient GraphQL error that pushed the squash commit but left this PR's merged-flag unset; closing manually.

@vidaks vidaks closed this Jun 16, 2026
@vidaks vidaks deleted the review-fixes-2026-06-16 branch June 16, 2026 13:27
vidaks added a commit that referenced this pull request Jul 9, 2026
A design/architecture/implementation review surfaced a set of small but real
findings; this addresses them together.

- Store leaked SQLite connections: _conn() used the bare sqlite3 context
  manager, which commits but never closes the handle — an fd leak and a
  ResourceWarning storm in the long-running maintain process. _conn is now a
  contextmanager that commits AND closes.

- COLOPHON_DB and COLOPHON_REPORTS were documented (README, .env.example) but
  never read; the changelog DB and reports always used repo-relative paths.
  Both now override their defaults (unset => unchanged behaviour). The five
  duplicated report-directory blocks in the CLI collapsed into one helper.

- CI compiled and linted but never ran the 69-test network-free suite, despite
  the docs stating it gates CI. Added the unittest step across Python 3.9-3.12.

- Grimmory.token() let a raw urllib error (a bad X-Edda-Proxy-Auth secret or an
  unreachable server) escape past the CLI's GrimmoryError/PreconditionError
  handler as a traceback; it is now a GrimmoryError naming the endpoint.

- Corrected the dedup docs: the README/CHANGELOG described an automated
  "collapse duplicate records" feature that was never built. The audit only
  identifies duplicate groups and suggests a keeper.

- Removed a dead, misleading MAX_PER_RUN constant from heal.py (the per-run cap
  lives in backfill.py; heal_book enforces none).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant