[eldad-caura-ai]

Why

The four open findings from audit batch B28 (database upgrade and readiness). All are in the storage service.

• L-37: a request body that is not valid UTF-8 fails with UnicodeDecodeError, which the app-wide 422 handler for JSONDecodeError does not catch. Every router that parses its body inline answers 500, which core-api turns into a retryable 503.
• L-139: a reader configured from the README (CORE_STORAGE_ROLE=reader, READ_DATABASE_URL set) sends only its get_read_session call sites to the read database. The rest, about two thirds, open the writer engine, still on DATABASE_URL. Shutdown disposes only the writer engine, and on a reader nothing.
• L-145: preflight_012 promises exit 0 for a database already past 012 but decides on the row count alone, so a re-embedded database past 012 is reported destructive (exit 1).
• L-97 (the remaining half; the JSON types were fixed in fix(models): declare the JSON columns migration 001 creates as json #1794): 17 columns migration 001 created nullable are declared NOT NULL by their models, so autogenerate proposes SET NOT NULL and create_all test schemas refuse rows production stores.

What changes

• L-37: an app-wide UnicodeDecodeError handler answers 422, as the malformed-JSON handler does, but only for a decode raised inside Request.json, that is, while parsing the body. Any other decode failure is re-raised and stays a 500. The two routers that caught it locally (tenant_suppression, preview) now rely on it.
• L-139: on a reader given READ_DATABASE_URL, get_engine() returns the read engine, so every session opens there and the reader keeps one pool. This is what config.py already describes and what the SaaS reader gets by setting DATABASE_URL to the read pool; a reader without READ_DATABASE_URL, and the writer and hybrid roles, are unchanged. Shutdown calls a new dispose_engines(), which closes every engine the process built, on any role. The README says what READ_DATABASE_URL does on each role.
• L-145: preflight_012 places the head on the migration chain before counting. At 012 or later it reports 012 applied, counts nothing and exits 0; the JSON report gains migration_applied. A head before 012, or one this checkout's chain does not know, is counted as before.
• L-97: the models declare nullable=True on the 17 columns migration 001 created nullable, as fix(models): declare the JSON columns migration 001 creates as json #1794 did for their JSON types. Annotations are unchanged. A new compare_metadata check fails on any modify_nullable diff. NON_NULLABLE_UPDATE_FIELDS keeps weight, so a PATCH still refuses a null weight; its drift test now says why. The database is not changed. Eldad decided against a NOT NULL migration; a follow-up PR will stop storage's bulk insert writing an explicit NULL weight or created_at, the one path that stores a NULL today.
• test_schema_drift_report's lifespan test now checks that shutdown calls dispose_engines().

Rollout

No migration. Readers given READ_DATABASE_URL stop connecting to DATABASE_URL, so a reader whose read database is a replica serves every query from it.

Evidence

1. Tests alone (b2520b6, run 38136238575): the storage suite failed exactly the 9 new cases, each on the gap it pins; 952 passed.
   • L-37: a non-UTF-8 body to POST /agents and /agents/create-only answered 500. The two routers with local catches already answered 422 and passed.
   • L-139: a reader's get_session bound to primary.invalid, and shutdown disposed neither engine on any role. The control (writer and hybrid keep the primary) passed.
   • L-145: a database at 065, and one at 012, exited 1. The control (011 with vectors exits 1) passed.
   • L-97: the check listed exactly the 17 columns, each schema NULL and model NOT NULL.
2. The fix (5e945fc): passed CI (run 38137030960), as did Eldad's rebase onto main, 6fd872e (run 38137217222).
3. Review round 1: Claude Code Review found one Medium issue: the UnicodeDecodeError handler also answered 422 for decode failures unrelated to the body. Its test alone (7f7db5f, run 38137284764) failed on that handler with 422; 961 passed. The fix, 03d1b29: CI pending. Claude Code Review found no issues on it.

🤖 Generated with Claude Code

[github-actions]

Claude Code Review — skipped: draft PR

[github-actions]

🤖 Review by Claude Code

Summary

This PR does four things. It makes the model nullability match migration 001 (L-97). It sends every reader session to READ_DATABASE_URL and disposes both engines on shutdown (L-139). It moves the UTF-8 body 422 into an app-wide handler (L-37). It makes preflight_012 recognise databases already past 012 (L-145). The nullability, reader-engine and preflight changes look correct. One issue is in the new exception handler.

Medium/Low Issues

App-wide UnicodeDecodeError handler turns server-side decode failures into a misleading client 422

Severity: Medium
File: core-storage-api/src/core_storage_api/app.py:267-276
Problem: The handler is registered for every UnicodeDecodeError raised anywhere in the app, not only the one from request.json(). A decode failure on the server side, such as decoding bytes from a DB driver, a file or a secret, would be reported as a 422 "request body must be valid JSON". That is a caller error, so core-api would treat it as non-retryable. It would also stop appearing as a 500 for alerting.

🤖 Claude Code Prompt

In /workspace/core-storage-api/src/core_storage_api/app.py, around lines 267-276, `_malformed_json_handler` is registered with `@app.exception_handler(UnicodeDecodeError)` as well as `json.JSONDecodeError`. This makes every UnicodeDecodeError in the whole app, including ones unrelated to the request body, return 422 "request body must be valid JSON". Server-side decode faults are therefore hidden as client errors.

Change it so only body-parsing failures get the 422. Two ways to do that:
- Keep the JSONDecodeError handler. Add a small helper used by the routers that call `await request.json()`, or a dependency or middleware, that catches UnicodeDecodeError only around the body read and parse and raises the 422 there.
- Or keep an app-level handler for UnicodeDecodeError, but have it answer 422 only when the exception came from the request-body read. For example, mark it at the parse site. Otherwise, log the error and return the normal 500.

Keep the existing behaviour for the routers that parse their body inline (agents, agents/create-only, tenant-suppression, preview/tenant-counts). Also keep the 422 body {"detail": "request body must be valid JSON"}.

---

Reviewed by claude-sonnet-5-5 · cost $0.27447375

[eldad-caura-ai]

Addressed the Medium finding from the review, the app-wide UnicodeDecodeError handler. The fix is in 03d1b29.

• What changed: UnicodeDecodeError has its own handler again, separate from the JSONDecodeError one. It answers 422 only when the exception was raised inside Request.json, that is, while parsing the request body (_raised_parsing_the_body walks the traceback for that frame). Any other decode failure is re-raised, so it stays the 500 it was before this PR. The JSONDecodeError handler is unchanged from main.
• Test first: test_l37_a_decode_failure_past_the_body_stays_a_500 makes a storage method raise UnicodeDecodeError while serving a valid request. Pushed alone as 7f7db5f, it failed on the previous handler with a 422 (run 38137284764, 1 failed, 961 passed). The non-UTF-8 body cases still expect 422, on the routers that parse inline and on the two that used to catch it locally.

🤖 Generated with Claude Code

[eldad-caura-ai]

@claude please review

[github-actions]

🤖 Review by Claude Code

Claude Code Review ✅ No issues found.

---

Reviewed by claude-sonnet-5-5 · cost $0.26764675

[eldad-caura-ai]

@erni-a please review