Skip to content

bug: mid-string 'Z' in agent-supplied fire_at passes validation then 500s in the parser #1831

Description

@AndriiPasternak31

Summary

fire_at is agent-supplied (MCP set_reminder, z.string().optional()). A string containing a Z in the middle passes Pydantic validation and then raises out of the parser, producing HTTP 500 where 400 belongs.

Found by the property test_P7_validator_acceptance_implies_the_parser_can_parse in #1771 slice b (PR #1826), shipped there as a strict=True xfail with no product fix (that PR's AC forbids product-code change).

Root cause — two normalizers that disagree

Code Behaviour
Validator src/backend/models.py:2134datetime.fromisoformat(v.replace("Z", "+00:00")) replaces every Z
Parser src/backend/utils/helpers.py:117-119if timestamp.endswith('Z'): timestamp = timestamp[:-1] + '+00:00' strips only a trailing Z

So 2026-01-15T10:30Z:00 normalizes, under the validator, to a string fromisoformat accepts — but the parser leaves the mid-string Z in place and raises ValueError.

Verified reproduction

Measured directly (not inferred), on the interpreters that matter:

Python Inputs reproducing
3.13 (prod image, docker/backend/Dockerfile:1) 4 / 4
3.12 4 / 4
3.14 2 / 4 — fromisoformat tightened

The four inputs:

2026-01-15T10Z:30:00
2026-01-15T10:30Z:00
2026-01-15 10Z:30:00
2026-01-15 10:30Z:00

On 3.14 only the two 10:30Z:00 shapes still reproduce. The xfail's example set therefore narrows but stays non-empty if Trinity ever moves to 3.14 — the strict xfail keeps holding.

Why it reaches the client as a 500

  • reminder_service._resolve_fire_at:38-39 calls parse_iso_timestamp(data.fire_at) with no try/except, under a comment asserting the very invariant this falsifies ("fire_at already validated ISO-8601 by ReminderCreate").
  • routers/reminders.py:120-137 catches except HTTPException: only, so the bare ValueError propagates to FastAPI.

Amplifier — needs confirmation against a live backend

The same escape appears to skip idempotency_service.fail(idem), stranding the in_flight claim. Because the key is derived over the raw input under the 24h default TTL, an identical retry would then get 409 "still being processed" for up to 24 hours instead of a second 500. This is a code-read inference and has not been executed — verify before relying on it.

Suggested fix

Make the two normalizers agree. Either give parse_iso_timestamp the strict contract and have the validator use it, or (minimal) tighten the validator to strip only a trailing Z. Then delete the strict=True xfail in tests/unit/test_1771b_timestamp_helpers_properties.py:529 — while it is strict, a fix makes it XPASS and fail the suite, which is the intended alarm, not a broken test.

There is a second replace("Z", "+00:00") at src/backend/models.py:358 worth auditing for the same divergence.

Found via #1771 (PR #1826).

Metadata

Metadata

Assignees

Labels

complexity-lowComplexity: low (board points 1-3)priority-p2Importantstatus-in-devMerged to dev, awaiting release cut to mainstatus-readyGreenlit and ready for development (vetted; counterpart to status-incubating)theme-reliabilityTheme: Reliabilitytype-bugBug fix

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions