fix(models): delegate fire_at ISO validation to parse_iso_timestamp (#1831)

[vybe]

Fixes #1831

What broke

ReminderCreate._check_fire_at_iso and reminder_service._resolve_fire_at were two hand-maintained normalizers that disagreed. The validator did v.replace("Z", "+00:00") — replacing every Z — while utils.helpers.parse_iso_timestamp strips only a trailing Z. So a fire_at carrying a mid-string Z passed validation, then hit the service's unguarded parse_iso_timestamp(data.fire_at) call and escaped as an uncaught ValueError → HTTP 500. fire_at is agent-supplied through the MCP set_reminder tool (z.string().optional()), so the malformed value arrives from outside the platform.

The fix

src/backend/models.py — the validator now calls the same parser the service uses, so "the validator accepted it" and "the parser can parse it" are the same statement rather than two rules that happened to agree. ~2 lines of behaviour plus the comments that keep it that way. models.py:13 already imported from utils.helpers (stdlib-only), so there is no new dependency and no circular-import risk.

This also makes PR #1826's strict=True xfail — test_P7_validator_acceptance_implies_the_parser_can_parse — hold by construction; the xfail is deleted in the same commit (a strict XPASS is a CI failure).

---

Read this before reviewing: five things that are decisions, not gaps

1. The resolved status code is 422, not the issue title's "400" — and that is NOT an unmet AC

A @field_validator raising ValueError surfaces as RequestValidationError → 422. Verified first-hand: no RequestValidationError handler and no exception_handler/add_exception_handler override exists in src/backend/main.py, so FastAPI's default applies.

422 is also this endpoint's own documented field-level convention (docs/memory/feature-flows/agent-self-reminders.md:49, and architecture.md:546 already records message ≤4000 (422)). Forcing 400 would mean moving validation into the service — reintroducing the very validator/parser divergence this issue closes — and would land the rejection after idempotency_service.begin(), i.e. after a claim has already been staked.

2. The issue's second ask was performed — outcome: latent, deliberately deferred

The issue also asks to "audit models.py:358 replace("Z","+00:00") for the same divergence." That audit ran. Result: the bug shape is present but latent — those values are never re-parsed.

_validate_iso8601 (models.py:360-368) backs ReportCreate.period_start/period_end. Grep-confirmed end to end: they are stored as TEXT (db/schema.py:499-500, migrations/versions/0006_agent_reports.py:38-39), passed through routers/reports.py:96-97 → database.py:1502 → db/reports.py as opaque strings, and rendered raw. Nothing calls fromisoformat on them anywhere but the validator itself, so there is no second parser to diverge from — no live defect, unlike fire_at.

Saying this plainly so it is not read as an unmet AC: the second ask was answered, not skipped. It is carried as follow-ups (1) and (2) below — and (2) is arguably the more serious defect on that same model.

3. Scope honesty — this kills this instance, not the class

Rejection now happens in the Pydantic validator, i.e. before idempotency_service.begin(). That removes this instance of the stranded-in_flight → 24h-409 wedge: previously a malformed fire_at claimed an idempotency key, then 500'd out without complete() or release(), so every retry for the next 24 hours got a 409.

It does not fix the class. Any other non-HTTPException escape between begin() and the terminal — a sqlite3 error, a Redis error, a future AttributeError — strands identically. And it must not be "fixed" with a blanket except Exception around the dispatch; see the standing comment at routers/reminders.py:157-161. The correct shape is a narrow pre-insert rescue. That is worth its own issue (follow-up 3).

4. No regression, argued BY CONSTRUCTION — not by corpus

A corpus argument is precisely what had a gap here in the first place, so the claim is structural:

newly_rejected  ≡  {old validator accepts}  ∧  {parse_iso_timestamp raises}
                ≡  exactly today's HTTP-500 set

An input in that set does not work today — it 500s. Therefore no input that works today stops working.

Independently re-verified at ship time over a freshly generated 658-input corpus (canonical shapes × every insertion index × {Z, z, T, X, space}, plus garbage): newly_rejected == broken_today → True; inputs that work end-to-end today → 19, of which 0 are now rejected.

5. The deliberate loosening: uppercase Z as the date/time separator

2026-01-15Z10:30:00 is now accepted. Lowercase z and X in that position already were — uppercase Z was rejected solely as an artifact of the replace bug ("…15Z10:30:00".replace("Z","+00:00") → …15+00:0010:30:00, garbage). Measured at ship time:

input	old validator	new validator	parser
2026-01-15Z10:30:00	reject	accept	ok
2026-01-15z10:30:00	accept	accept	ok
2026-01-15X10:30:00	accept	accept	ok

This is a loosening, not a regression, and it is pinned as intended by test_1831_uppercase_z_separator_now_accepted.

---

Scope of the divergence class

The issue names four inputs. Measured, the class is seven inputs wide on py3.13 (the prod interpreter), narrowing to four on py3.14 as fromisoformat tightens. The corpus was additionally measured portable to py3.11 — the CI interpreter — with byte-identical results.

Interpreter-parity gap worth your attention (pre-existing, not introduced by this PR)

.github/workflows/backend-unit-test.yml runs Python 3.11 (three setup-python steps, all '3.11') while the prod image is Python 3.13 (docker/backend/Dockerfile:1 → FROM python:3.13-slim), and verify-local builds/tests the 3.13 image. A green verify-local therefore does not cover the CI interpreter, and vice versa — and fromisoformat acceptance is exactly the kind of behaviour that moves between minor versions.

Closed for this change by re-running the entire corpus on 3.11.14: all 7 mid-Z rejected, all 7 Z-separator accepted, all 7 canonical accepted, all 5 garbage rejected — byte-identical to 3.13 and 3.14. (This run also observed verify-local's own venv on 3.14.3 — three interpreters gating one codebase.) Logged as follow-up 6.

---

return v byte-identity is load-bearing

The validator returns v unchanged — never canonicalized. The create-idempotency key hashes raw_fire_spec() == f"fire_at={self.fire_at}" (models.py:2198-2204) → derive_reminder_key → idempotency_service.begin(). Canonicalizing here would fork the derived key and double-create on a client retry (Invariant #18). A dedicated test pins it (test_1831_validator_does_not_canonicalize_fire_at), and a comment at the return says why.

Pre-existing and by design, so review doesn't raise it as new: keying on the raw spec means two spellings of the same instant hash differently. That equivalence class was already unbounded; the Z-separator family adds three members to it. Unchanged by this PR — the alternative (key on the resolved instant) is what Invariant #18's raw-input rule deliberately rejects.

---

The _validate_iso8601 fence: HELD, on scope grounds only

Stating this plainly: the original merge-conflict justification for the fence was measured false and is retracted. PR #1838's hunk ends around line 354 and the target is line 365 — it would merge cleanly.

The fence is held on the remaining merits: engineering proved those values are never re-parsed (latent, not live — see #2 above), and the more serious defect on that same model is follow-up 2 below. Fixing _validate_iso8601 alone would ship the less important half of a two-part fix, on a field another feature owns.

Consequently the proposed "fromisoformat" not in models.py static parity guard is deliberately not shipped — with the fence held it would fail immediately against models.py:365. It belongs with follow-up 1.

---

Test evidence

Run	Result
Affected-file neighbourhood, 5 suites (ship-time re-run)	190 passed
— same command on a pristine origin/dev baseline	133 passed, 1 xfailed (the xfail is P7)
P7 property	PASSED — 172 passing, 0 failing, 0 invalid, 20.93% validator accepted
Non-vacuity: revert the product fix, keep the tests	29 failed, 27 passed; P7 1 failed — the tests provably detect the bug
Blast radius: all 55 unit files importing models	1060 passed, 1 skipped
CI-shaped run under all three pytest-randomly CI seeds	190 passed each
120,000-mutation fuzz over the validator	no exception type escapes (ValueError, AttributeError)
Full tests/unit (WAVE-4 verify-local)	1 failed, 6088 passed, 18 skipped — see below

pytest tests/unit/test_1771b_timestamp_helpers_properties.py \
       tests/unit/test_1771b_timestamp_helpers_edges.py \
       tests/unit/test_1296_reminders.py \
       tests/unit/test_1806_reminder_autonomy_hold.py \
       tests/unit/test_1713_scheduler_utils_parity.py -q -rs

Six new test_1831_* regression tests in tests/unit/test_1296_reminders.py (56 cases of the file's 89): the seven measured mid-string-Z inputs rejected · fire_at/raw_fire_spec() byte-identity · validator ⟹ parser one-directionally over a fixed corpus · the uppercase-Z loosening pinned as intended · the XOR guard proven load-bearing against parse_iso_timestamp(None) · rejection landing before idempotency_service.begin().

P7's non-vacuity now has a mechanical floor — and why it needed one

The 20.93% validator accepted figure was degenerate. Measured across 3.11/3.13/3.14, 100% of accepted draws came from i >= len(base) — a plain trailing-Z append — and zero true mid-string insertions were accepted. So "21%" read as coverage while actually exercising three canonical strings. A concrete drift path existed: narrowing st.integers(max_value=27) to the base lengths — a plausible magic-number cleanup — would silently drop acceptance to 0% and leave the property green and worthless.

Fixed with a fifth, accepted @example ("2026-01-15Z10:30:00") as a floor, verified at ship time to be accepted by the validator, so the assertion body executes at least once regardless of what the strategy later becomes.

Separately, the # NOT assume() warning was moved out of the docstring to the return site: reverting the early-return to assume(_validator_accepts(raw)) reintroduces a deterministic FailedHealthCheck: filter_too_much (~79% of draws now filtered; the shared ci profile suppresses only too_slow/data_too_large) and re-arms source-digest seed sensitivity under derandomize=True. A comment two paragraphs into a docstring does not survive that edit; one at the return does.

---

Verification honesty

• verify-local ran --skip-agent — a host constraint, not a passing result: origin/dev is global agent-network mode and the operator's live dev stack owns trinity-agent-network, so the agent stage hard-refuses.
• Docker stages pass (the ones that matter here): build + import-smoke 16s — the load-bearing gate, since this branch adds a top-level from utils.helpers import parse_iso_timestamp to models.py, which is imported on the backend startup path; boot + health 13s; integration 70 passed.
• The one unit red is proven pre-existing, not assumed. tests/unit/test_1069_voip_call_path_param.py::TestFlatPathParams::test_flat_path_params_are_agent_name_not_name reproduces identically on a pristine git archive origin/dev @ 8e924526 in the same venv. Repo-level root cause: fastapi>=0.115.0 is a floor, not a pin, so any fresh venv resolves 0.141.1, where get_flat_dependant no longer exists. It will red for every contributor building a clean env — its own issue, and must not be attributed to this PR.

---

⚠️ Merge-order warning — do not let auto-merge resolve this

docs/memory/learnings.md will conflict with the PR for #1849. Both append a new ## 2026-08-01 — pitfall — … block at EOF, patching the identical hunk @@ -230,3 +230,7 @@ with the same three trailing context lines. Git's 3-way merge cannot auto-resolve two insertions at one anchor.

Merge #1849 first (it is P1); this PR then rebases and keeps both blocks (either order). Resolve by hand.

---

Follow-ups (recorded here, deliberately not filed)

1. _validate_iso8601 still carries the bug shape. models.py:360-368 keeps value.replace("Z", "+00:00"). Ship it together with (2), plus the "fromisoformat" not in models.py static parity guard, as one reports-owned issue.
2. ReportCreate._check_period_order compares period_start > period_end LEXICOGRAPHICALLY on raw, un-normalized strings (models.py:402). This is exactly the mixed-shape trap Invariant #16 exists to prevent — 2026-01-15T10:00:00Z vs 2026-01-15 10:00:00+00:00 compare wrong (T > space). Arguably more real than (1).
3. The stranded-idempotency-claim class (see §3). Any non-HTTPException escape between begin() and the terminal strands a claim for 24h. Needs a narrow pre-insert rescue — not a blanket except Exception.
4. .replace("Z", "+00:00") at 11 further backend sites. All verified to consume platform-written input → hygiene, not urgency.
5. _validator_accepts catches bare Exception (pre-existing, inherited from test(timestamps): edge-case and property coverage for the ISO helpers and their scheduler mirror #1826) — narrow it to ValidationError.
6. The CI-3.11 vs prod-3.13 interpreter gap (see above) — a standing, repo-wide exposure.

Plus, from /validate-pr, disclosed rather than "fixed":

• No docs/memory/feature-flows.md Recent Updates row was added. The check requires one only for a new flow (this is an update), CLAUDE.md Rule fix: add missing logging_config.py to backend Dockerfile #4 scopes a bug fix to a commit message, and adding one would manufacture a second guaranteed merge conflict with bug(whatsapp): inbound media download always fails — SSRF allowlist rejects Twilio's own media CDN #1932, which already appended at the same top-of-table anchor.
• architecture.md:1079's error list gains no 422 note. The whole Agent Self-Reminders section was read: it makes no claim about the normalization algorithm and no claim that a malformed fire_at returns 400, so nothing in it is falsified.
• agent-self-reminders.md does not match the canonical flow template (## Tests not ## Testing, no Frontend Layer — there is no frontend surface). Pre-existing from feat: agent self-reminders — agent-callable one-shot deferred self-trigger ("remind me to do X later") #1296; both edited sections exist. Restructuring would be scope creep on a P2 bug fix.

---

🤖 Generated with Claude Code

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[AndriiPasternak31]

Review — /review + /validate-pr, second pass

Read-only against origin/dev @ 8e924526. Head ff0004b4 unchanged since the first pass. Reviewed together with #1938 — the two are coupled, but only by merge order (see below).

Verdict: APPROVE-grade diff. No code change requested. Two behavioural lines that replace a hand-maintained normalizer with the exact parser its only consumer already calls, making "the validator accepted it" and "the parser can parse it" the same statement by construction rather than two rules that happened to agree. Posting as a comment rather than an approval only because the discovered follow-ups are still unfiled.

Re-verified this pass and holding:

• Invariant #16 / scheduler mirror intact. src/backend/utils/helpers.py and src/scheduler/utils.py are both absent from the diff (empty git diff on each) — the PR adds a caller, nothing the mirror must agree on moved. And the widened class is structurally unreachable from the mirror anyway: reminder_service.py:138 persists to_utc_iso(fire_dt), so parse_scheduler_ts never sees a Z-separator string.
• Invariant #14 intact. No file under routers/ appears in the diff at all, so no BaseModel was added there; the test_models_centralized.py guard surface is untouched.
• Coupling with #1938 is merge-order only. comm -12 on the two PRs' file lists returns exactly one shared path — docs/memory/learnings.md. No shared symbol, import, or behavioural ordering.

---

The adjacent bug this PR surfaced is real — please file it

Follow-up 2 in the body (models.py:402) is not hypothetical. I reproduced it, and it fails in both directions.

src/backend/models.py:400-404 compares raw, un-normalized ISO strings — _validate_iso8601 (models.py:360-368) validates and then return value unchanged, so the separator and offset spelling are entirely caller-controlled:

@model_validator(mode="after")
def _check_period_order(self) -> "ReportCreate":
    if self.period_start and self.period_end and self.period_start > self.period_end:
        raise ValueError("period_start must be <= period_end")

case                             lexical      real   verdict
T-vs-space separator                True     False   FALSE 422 (valid range rejected)
T-vs-space, INVERTED               False      True   INVERTED RANGE ACCEPTED
same instant, diff offset           True     False   FALSE 422 (valid range rejected)
cross-midnight w/ offset           False      True   INVERTED RANGE ACCEPTED
control: both canonical             False     False   ok

Concretely — both values pass the field validator in each case:

• period_start='2026-01-15 23:00:00+00:00', period_end='2026-01-15T09:00:00Z' → ' ' < 'T', order check passes, a 14-hour-inverted range is persisted to agent_reports.
• period_start='2026-01-15T10:00:00+00:00', period_end='2026-01-15T05:00:00-05:00' → the same instant, rejected 422.

Blast radius is contained and the body's audit conclusion is correct — I independently confirmed the values are opaque TEXT pass-through across all 28 period_start/period_end references, none of which call fromisoformat/parse_iso_timestamp/strptime. So the consequence is a wrong 422 or a silently-inverted stored range, not a crash. Genuinely latent.

This is out of scope for this PR and should not be fixed in it — same trap (Invariant #16), different surface, and #1831 is a 2-line P2. But three tracker searches (_validate_iso8601, the period compare, 1831 follow-up) all return empty, and everything created since 2026-08-01 (#1931–#1933) is unrelated. Suggested framing: type-bug, priority-p2, theme-reliability, complexity-low.

Follow-up 3 (the stranded in_flight idempotency claim) is worth filing on the same pass.

---

Merge order

Confirmed by git merge-tree, not inferred:

#1938 into current dev     -> clean
#1940 into (dev + #1938)   -> CONFLICT, docs/memory/learnings.md ONLY

The body's merge-order warning is correct but under-scoped: four PRs append at the same EOF hunk, not two — #1938, #1911 and this one at @@ -230,3 +230,7 @@, #1913 at @@ -230,3 +230,11 @@. So the conflict is guaranteed in every ordering, and hand-resolution is required regardless of who lands first.

It is, however, a trivial conflict: one file, both sides disjoint appends to an append-only ledger. Resolution is concatenate, keep both, in merge order — no semantic decision, no content to reconcile. The only real hazard is an auto-resolver or a careless --ours/--theirs dropping an entry. Verify after each with:

git show dev:docs/memory/learnings.md | grep -c '^## 20'

Suggest letting #1938 (P1, currently conflict-free) land first, then rebasing this one.

Pre-merge checklist

• File follow-up 2 (models.py:402 lexicographic period compare — verified live above) as type-bug
• File follow-up 3 (stranded in_flight idempotency claim)
• Correct the merge-order note: #1938, #1911 and #1913 share the anchor, not #1849 alone
• Optional: reword "the class is seven inputs wide" → "seven pinned exemplars of an unbounded class" (a wider corpus finds 10 on py3.13; doesn't weaken the fix, which is by construction, but the prose reads as a closed set)
• Second reviewer per SOC 2 (this is a comment, not an approval)

_{Full working notes, including the reproduction script behind the table above, are in my local review record. Happy to paste it into the thread.}

[AndriiPasternak31]

Approving — ff0004b4

Converting my earlier review to an approval. Nothing in the diff changed; my
re-verification of Invariant #16 (scheduler mirror untouched, and structurally
unreachable from this path since reminder_service.py:138 persists
to_utc_iso(fire_dt)) and Invariant #14 (no routers/ file in the diff) stands.

The two behavioural lines replace a hand-maintained normalizer with the exact
parser its only consumer already calls, so "the validator accepted it" and "the
parser can parse it" become the same statement by construction. That's the right
shape of fix.

The open items are administrative, not code, so they should not gate the diff:

• Follow-up 2 (models.py:402 lexicographic period compare) and follow-up 3
  (stranded in_flight idempotency claim) are both pre-existing defects
  surfaced by this review — neither is introduced or worsened here. They belong
  in the tracker, not in this PR. Still worth filing.
• The merge-order note in the body under-scopes the learnings.md anchor:
  #1938, #1911 and #1913 share it, not #1849 alone. The conflict is guaranteed
  in every ordering and is a keep-both concatenation of an append-only ledger —
  no semantic decision. Verify after resolving with
  git show dev:docs/memory/learnings.md | grep -c '^## 20'.

Note dev has moved twice today (#1935 609f8dff, #1934 f5a21624); I
re-simulated this branch against the new tip and it still merges clean.