ReviewStateValidator does not enforce the schema's date-time format on any timestamp field #474

New issue

Closed

opened 2026-08-28 07:09:59 +00:00 by mph · 1 comment

mph commented

2026-08-28 07:09:59 +00:00

Owner

Found triaging IW-437 (raised in phase 3 code review; pre-existing, deliberately not fixed there).

schemas/review-state.schema.json declares format: "date-time" for the card's timestamp fields, ci_wait_started included (schemas/review-state.schema.json:98-103).

ReviewStateValidator.validate (core/model/ReviewStateValidator.scala:274-281) only checks that the field is present and is a string. It never parses it. So "not-a-date" passes validation, and the schema's declared format is documentation that nothing checks.

SCOPE. This is a pre-existing pattern shared by every timestamp field on the card, last_updated included. It is not specific to ci_wait_started and was not introduced by IW-437.

WHY IT NOW MATTERS MORE. ci_wait_started gates a safety timeout. An unparseable value there does not fail loudly at validation - it silently resets the CI wait budget on every resumed invocation, so a capped wait becomes unbounded. See #473 for that fallback's semantics. Enforcing the format here is what stops a bad value reaching startOfWait through an ordinary write rather than through file corruption.

PROPOSED FIX. Parse and reject on failure in ReviewStateValidator, for every field the schema declares as date-time - not only ci_wait_started.

Kept separate from #473 deliberately: different file, different scope (all timestamp fields vs one field's fallback semantics), and either can be fixed or deferred independently of the other.

Found triaging IW-437 (raised in phase 3 code review; pre-existing, deliberately not fixed there). schemas/review-state.schema.json declares format: "date-time" for the card's timestamp fields, ci_wait_started included (schemas/review-state.schema.json:98-103). ReviewStateValidator.validate (core/model/ReviewStateValidator.scala:274-281) only checks that the field is present and is a string. It never parses it. So "not-a-date" passes validation, and the schema's declared format is documentation that nothing checks. SCOPE. This is a pre-existing pattern shared by every timestamp field on the card, last_updated included. It is not specific to ci_wait_started and was not introduced by IW-437. WHY IT NOW MATTERS MORE. ci_wait_started gates a safety timeout. An unparseable value there does not fail loudly at validation - it silently resets the CI wait budget on every resumed invocation, so a capped wait becomes unbounded. See #473 for that fallback's semantics. Enforcing the format here is what stops a bad value reaching startOfWait through an ordinary write rather than through file corruption. PROPOSED FIX. Parse and reject on failure in ReviewStateValidator, for every field the schema declares as date-time - not only ci_wait_started. Kept separate from #473 deliberately: different file, different scope (all timestamp fields vs one field's fallback semantics), and either can be fixed or deferred independently of the other.

mph referenced this issue

2026-08-28 07:10:08 +00:00

A corrupt or future ci_wait_started silently disables the CI wait budget #473

mph referenced this issue

2026-08-28 07:22:08 +00:00

IW-437: The phase CI gate finishes a phase without manual intervention #475

mph referenced this issue from a commit

2026-09-02 06:22:12 +00:00

fix(review-state): reject a timestamp nothing can read

mph referenced this issue

2026-09-02 06:22:57 +00:00

Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card #501

mph commented

2026-09-02 20:55:14 +00:00

Author

Owner

Fixed in PR #501 (merge d1eba9c4). ReviewStateValidator now parses every field the schema declares format: date-time with java.time.Instant.parse and rejects on failure, naming the field. This is the route that let a bad ci_wait_started reach startOfWait (#473).

Fixed in PR #501 (merge d1eba9c4). `ReviewStateValidator` now parses every field the schema declares `format: date-time` with `java.time.Instant.parse` and rejects on failure, naming the field. This is the route that let a bad `ci_wait_started` reach `startOfWait` (#473).

mph closed this issue

2026-09-02 20:55:14 +00:00