feat: GTFS-RT feed validation & E012 compliance fix

[diveshpatil9104]

Summary

• Fix E012: header timestamp now guaranteed >= all entity timestamps
• Add bearing (0-360) and speed (>= 0) ingest validation per GTFS-RT spec
• Add 21-test spec-validation suite covering all 12 MobilityData validator rules
• Fix pre-existing test quality issues (realistic timestamps, assert.Eventually)

Why

Milestone 1 exit criteria requires the feed to pass the https://github.com/MobilityData/gtfs-realtime-validator without errors. An audit found two violations:

1. E012 — Header timestamp could be less than entity timestamps when vehicles report future times (within 5-min ingest window). Fixed with max(now, maxEntityTimestamp).
2. E027 — No bearing/speed validation, so invalid values could reach the feed. Fixed with ingest range checks.

What Changed

• handlers.go: E012 fix in buildFeed(), bearing/speed validation in validate(), skip zero-timestamp vehicles
• feed_validation_test.go: New file — validateFeedCompliance helper + 21 tests for E001, E012, E026, E027, E038, E039, E048, E049, E050, E052, W001, W002
• handlers_test.go: E012 assertion, 6 rejection cases, boundary acceptance test, fix Timestamp: 100 and time.Sleep

Known Limitations

• E027: Zero-value ambiguity (0.0 = North vs not-provided) pending PR Preserve nullable bearing, speed, and accuracy fields using pointers #45
• E050: Ingest allows ±5min but E050 threshold is 60s — documented, not changed here

Summary by CodeRabbit

• Bug Fixes

  • Enforced bearing (0–360°) and non-negative speed validation.
  • Feed header timestamp now computed from entity timestamps; feeds skip vehicles with non-positive timestamps.

• Tests

  • Added a comprehensive GTFS-RT feed validation suite covering many rules, boundary conditions, and edge cases.
  • New tests for bearing/speed boundary handling and improved feed-building and staleness behavior checks.

[aaronbrethorst]

Divesh, this is a well-structured approach to GTFS-RT compliance — the E012 header timestamp fix is exactly right, the validateFeedCompliance test helper is a great reusable asset for catching regressions, and the test quality improvements (replacing Timestamp: 100 with time.Now().Unix() and time.Sleep with assert.Eventually) are welcome cleanups. The 21-test validation suite organized by MobilityData rule number makes it easy to verify coverage at a glance.

Before this can merge, there are a few things to address.

Critical Issues (2 found)

1. Merge conflicts with main. GitHub reports this PR as CONFLICTING. You'll need to rebase against main to resolve.

2. Branch is stale against PR #45 (*float64 pointer migration) — code will not compile after rebase. PR #45 changed Bearing, Speed, and Accuracy from float64 to *float64 on both LocationReport and VehicleState. This PR uses bare float64 throughout:

   • Validation (handlers.go:67-71): r.Bearing < 0 || r.Bearing > 360 and r.Speed < 0 won't compile when these are *float64. After rebase, these checks must be conditional on non-nil — a nil bearing/speed (field not provided) should be allowed, not rejected. The validation should be:

     if r.Bearing != nil && (*r.Bearing < 0 || *r.Bearing > 360) {
         return fmt.Errorf("bearing must be between 0 and 360 (inclusive)")
     }
     if r.Speed != nil && *r.Speed < 0 {
         return fmt.Errorf("speed must be non-negative")
     }

   • buildFeed() (handlers.go:201-202): proto.Float32(float32(v.Bearing)) and proto.Float32(float32(v.Speed)) use bare value access, but on main these are pointers with conditional emission (see PR #45's buildFeed changes that use nil guards). After rebase, you'll need to adopt the same nil-guard pattern.

   • feed_validation_test.go: All VehicleState literals use bare float64 fields (e.g., Bearing: 180, Speed: 8.5) which won't compile against *float64. These need to use the float64ptr() helper.

   • handlers_test.go: Same issue — the new validation test cases use Bearing: -1, Speed: -5, etc. on LocationReport, which will need float64ptr().

Important Issues (1 found)

1. E050 tension with ingest window is documented but may need a follow-up. The ingest window allows timestamps up to now+300s, but E050 rejects >now+60s. The PR correctly documents this in TestFeedValidation_E050_TimestampsNotFarFuture and in the "Known Limitations" section. However, this means a vehicle with a timestamp 2 minutes in the future will pass ingest validation, appear in the feed, and fail the MobilityData validator for E050. Since the goal is to pass the validator without errors, this gap should be tracked for follow-up — either tighten the ingest window to 60s (breaking change) or clamp entity timestamps in buildFeed() to min(ts, now+60).

Suggestions (1 found)

• Consider float64ptr() boundary tests for the pointer migration. Once you rebase and adopt *float64, you'll want test cases that verify nil bearing/speed passes validation (no rejection), explicit zero passes (bearing=0 is north, speed=0 is stationary), and out-of-range values are rejected. The current test cases cover the range checks well but will need the nil-is-valid case added.

Strengths

• E012 fix is clean and correct: headerTimestamp = max(now, maxEntityTimestamp) is a simple, correct solution
• Zero-timestamp vehicle skip prevents W001 violations in the feed with an appropriate slog.Warn
• validateFeedCompliance helper is a great reusable asset — it tests buildFeed() through the lens of every applicable MobilityData rule
• Test quality improvements: replacing hardcoded Timestamp: 100 with realistic time.Now().Unix() and flaky time.Sleep with assert.Eventually
• 21 well-organized tests covering E001, E012, E026, E027, E038, E039, E048, E049, E050, E052, W001, W002
• Protobuf round-trip test (TestFeedValidation_FeedSerializesCleanly) verifies the feed survives marshal/unmarshal

Recommended Action

1. Rebase against main to resolve merge conflicts
2. Adapt all bearing/speed code to the *float64 pointer types from PR #45
3. Add nil-bearing/speed validation test cases
4. Consider tracking the E050/ingest-window tension for follow-up
5. Re-run review after fixes

[diveshpatil9104]

hii aaron! I Rebased against main and resolved all conflicts. Changes:

• validate(): uses nil-conditional checks (if r.Bearing != nil && ...)
• buildFeed(): adopted nil-guard pattern from Preserve nullable bearing, speed, and accuracy fields using pointers #45
• All test literals: converted to float64ptr() in both test files
• Added nil bearing/speed boundary test cases (nil = not provided, passes validation)

[aaronbrethorst]

Divesh — the rebase landed cleanly and you addressed every critical item from the last pass. GitHub now reports the PR as MERGEABLE / CLEAN, the *float64 migration from #45 is fully adopted (validate() uses nil-conditional checks, buildFeed() uses the nil-guard emission pattern, every test literal moved to float64ptr()), and the new nil-bearing/speed boundary cases are exactly what I asked for. The build is clean, go vet is clean, and all tests pass. Nice turnaround.

There are two things to fix before this merges, and one to track for follow-up.

Critical Issues (1 found)

1. The E027 comment in the validator helper is factually false and contradicts the code three lines below it [feed_validation_test.go:93-96]. The comment reads:

   // buildFeed() always sets Bearing via proto.Float32(), so pos.Bearing
   // is never nil in current production output. The nil guard is retained
   // for forward-compatibility if Bearing becomes a pointer type.

   All three claims are wrong:

   • buildFeed() sets position.Bearing only when v.Bearing != nil (handlers.go:196-198), so pos.Bearing is nil whenever the source vehicle has no bearing.
   • You already have a test proving exactly that — TestBuildFeed_OmitsUnsetBearingAndSpeed (handlers_test.go:849) asserts pos.Bearing is nil when unset.
   • Bearing is already a pointer (*float64 on both LocationReport and VehicleState), so "if Bearing becomes a pointer type" describes a migration that already happened in #45.

   This matters because the if pos.Bearing != nil guard on line 97 is load-bearing, not dead code — the comment would mislead a future maintainer into removing it. Please replace with something accurate, e.g.:

   // E027: bearing 0..360 (inclusive per MobilityData E027 rule).
   // Bearing is optional; buildFeed() leaves pos.Bearing nil when the
   // source vehicle has no bearing, so the nil guard is required.

Important Issues (1 found)

1. validateFeedCompliance is never negative-tested — the core abstraction of this PR is itself unverified [feed_validation_test.go:20-121]. Every one of the ~17 call sites asserts assert.Empty(t, violations). No test ever feeds the helper an intentionally non-compliant feed and confirms it returns the expected violation. You could replace the entire helper body with return nil and nearly every test in the file would still pass. Two specific branches are provably never exercised:

   • E052 (duplicate IDs) — TestFeedValidation_E052_UniqueVehicleIDs passes three distinct IDs, so the if _, exists := seenIDs[id] branch (:46-50) never runs. buildFeed copies entity.Id straight from VehicleID (handlers.go:204), so duplicates are genuinely possible.
   • E050 (>60s future) — the entityTs > now+60 branch (:79, :116) is never hit with a triggering value.

   Add at least one assertion-of-rejection per rule the helper claims to enforce (a feed with duplicate IDs → expect an E052: violation; a feed with an entity at now+120 → expect an E050: violation). That converts the helper from "a checker we hope works" into "a checker we proved works."

Suggestions (1 found)

• Make the E050/ingest-window gap regression-visible [feed_validation_test.go:269-283]. To your credit, the gap is honestly documented in the comment — ingest accepts now+300s but E050 rejects >now+60s, so a vehicle legitimately accepted at now+120 produces a feed the real MobilityData validator flags E050 on (and the header inherits that timestamp via the E012 max logic). But "documented" isn't "tested" — the known-bad case lives only in a comment CI can't see. A t.Skip-flagged or explicitly-asserting test pinning "now+120 currently yields E050 — known limitation" would make the gap visible. I'll track this as a follow-up issue regardless (see below); it does not need to be solved in this PR.

Strengths

• E012 max(now, maxEntityTimestamp) logic is correct: the header is never dragged below now for all-past feeds, and the non-positive-timestamp skip runs before the comparison so bogus values can't corrupt the header.
• The uint64(v.Timestamp) conversion is safe precisely because the v.Timestamp <= 0 guard runs first (handlers.go:183-187).
• The bearing/speed validation tests are exemplary — real validate() calls, true boundary values (-0.001, 360.001, exactly 0, exactly 360), proper error-string assertions, and explicit nil-is-valid cases.
• TestFeedValidation_ZeroTimestampVehicleSkipped is a genuine behavioral test of the new skip logic, covering both zero and negative timestamps.
• TestFeedValidation_FeedSerializesCleanly adds real value with a protobuf round-trip.
• The test-quality cleanups (Timestamp: 100 → time.Now().Unix(), time.Sleep → assert.Eventually) are correct and necessary given the new timestamp-skew interactions.
• The E012 tests that assert directly on feed.Header.GetTimestamp() vs entity timestamps are the strongest in the file — independent of the helper.

Recommended Action

1. Fix the false E027 comment (feed_validation_test.go:93-96) — it's the only critical item.
2. Add at least one rejection-asserting test for validateFeedCompliance, exercising the E052 and E050 branches.
3. Consider the E050 skip/pin test (optional; tracked separately).
4. Re-run review after fixes.

Verdict: Request Changes.

[coderabbitai]

Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in.

Warning

Review limit reached

@diveshpatil9104, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 51 minutes and 16 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c994322-9a9e-4fae-a3a7-a82bedc10328

📥 Commits

Reviewing files that changed from the base of the PR and between bd1c3a3 and bff8244.

📒 Files selected for processing (3)

• feed_validation_test.go
• handlers.go
• handlers_test.go

📝 Walkthrough

Walkthrough

This PR enforces bearing and non-negative speed validation, refactors feed construction to compute header timestamps from entity timestamps while skipping non-positive entries, updates tests to use dynamic timestamps and polling, and adds a comprehensive feed compliance test suite for GTFS-RT VehiclePosition feeds.

Changes

GTFS-RT Validation and Feed Building

Layer / File(s)	Summary
Bearing and Speed Validation handlers.go, handlers_test.go, feed_validation_test.go	LocationReport.validate() enforces optional bearing (0–360 inclusive) and speed (non-negative). Tests cover boundary values, negative values, and HTTP POST integration cases exercising nil and boundary bearing/speed inputs.
Feed Building with Timestamp Refactoring handlers.go, handlers_test.go	buildFeed() now collects entities into a slice, skips vehicles with non-positive timestamps (logs a warning), sets entity Timestamp from a precomputed ts, and computes the feed header Timestamp as the maximum entity timestamp. Tests assert header timestamp >= max entity timestamp.
Test Infrastructure and Dynamic Timestamps handlers_test.go	Handler tests switch from fixed timestamp constants to time.Now().Unix() across several tests and replace fixed sleeps with assert.Eventually polling to wait for stale-entity exclusion.
Comprehensive Feed Compliance Validation feed_validation_test.go	Adds validateFeedCompliance() helper and ~23 focused and composite tests validating GTFS-RT header and entity rules: version/header timestamp/incrementality, POSIX-seconds timestamps, header>=entity timestamps, future-window checks, latitude/longitude bounds, optional bearing bounds, non-negative speed, unique entity IDs, prohibition of is_deleted for FULL_DATASET, empty/full feed cases, timestamp skipping, and protobuf round-trip compliance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: GTFS-RT feed validation & E012 compliance fix' directly addresses the two main objectives: GTFS-RT feed validation (the new 21-test validation suite) and E012 compliance (the header timestamp fix). It is specific, concise, and clearly summarizes the primary changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

handlers.go (1)

178-230: E012 header logic is correct, but headerTimestamp can advertise a future time and trip E050.

validate() accepts entity timestamps up to now + 5min (maxTimestampSkew), and buildFeed() sets the header to max(now, maxEntityTimestamp). A vehicle reporting, say, now+180s passes ingest, gets included, and drags the header to now+180s. The canonical validator treats future timestamps as an E050 error (the rule originates from "timestamps shouldn't be in the future (greater than now)"), so the published feed would fail E050 even though E012 is satisfied. This is the documented ingest-vs-E050 gap; flagging for visibility since it weakens the compliance posture this PR targets.

A follow-up option is to clamp/skip future entity timestamps at ingest (or cap the header at now only when no entity legitimately exceeds it) so the two windows stay consistent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@handlers.go` around lines 178 - 230, The headerTimestamp can be set in the
future (variable headerTimestamp in buildFeed), causing published feeds to
trigger E050 even though validate allows a small future skew; fix by clamping
the feed header to not be greater than now: keep the existing logic that
computes headerTimestamp from entity timestamps but after the loop ensure
headerTimestamp = min(headerTimestamp, now). Alternatively, if you prefer to
preserve entity timestamps in the feed but avoid E050, clamp per-entity
timestamps when constructing the FeedEntity (use tsClamped = min(ts, now) for
the Timestamp field) and still use the original ts when deciding
ingest/validation; change the code in buildFeed where headerTimestamp and
entity.Timestamp are set (referencing headerTimestamp, v.Timestamp, and the
FeedEntity/VehiclePosition construction) to implement one of these two
approaches.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@handlers_test.go`:
- Around line 110-113: The E012 test uses fixed past timestamps so buildFeed()
always sets header to now and the assertion passes trivially; change the
vehicle/entity timestamp constants used in the test (the two values around lines
asserting entity timestamps and the header check using
feed.Header.GetTimestamp()) to be computed relative to now (e.g., now and now+N)
so the larger entity timestamp can legitimately drive the header, and update the
downstream entity-timestamp assertions and the header assertion to compare
against those now-relative values to properly exercise the max-entity logic
(E012) in buildFeed().

---

Nitpick comments:
In `@handlers.go`:
- Around line 178-230: The headerTimestamp can be set in the future (variable
headerTimestamp in buildFeed), causing published feeds to trigger E050 even
though validate allows a small future skew; fix by clamping the feed header to
not be greater than now: keep the existing logic that computes headerTimestamp
from entity timestamps but after the loop ensure headerTimestamp =
min(headerTimestamp, now). Alternatively, if you prefer to preserve entity
timestamps in the feed but avoid E050, clamp per-entity timestamps when
constructing the FeedEntity (use tsClamped = min(ts, now) for the Timestamp
field) and still use the original ts when deciding ingest/validation; change the
code in buildFeed where headerTimestamp and entity.Timestamp are set
(referencing headerTimestamp, v.Timestamp, and the FeedEntity/VehiclePosition
construction) to implement one of these two approaches.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a4652cfe-b621-4eba-a479-741ba830a8a3

📥 Commits

Reviewing files that changed from the base of the PR and between 23e535b and a866733.

📒 Files selected for processing (3)

• feed_validation_test.go
• handlers.go
• handlers_test.go

[aaronbrethorst]

Divesh — clean turnaround. Every item from the last pass is fully resolved: the E027 nil-guard comment now accurately describes the optional-bearing behavior, TestFeedValidation_E050_RejectsEntityFarInFuture and TestFeedValidation_E052_RejectsDuplicateIDs prove those two branches actually fire (the E052 test's manual-entity-injection pattern is exactly right), and you even pinned the ingest/E050 gap as a t.Skip marker — which was only a suggestion. Build, go vet, gofmt, and the full test suite are all clean.

One thing keeps this from merging, and it's a near-mirror of the E027 issue I raised last time.

Critical Issues (0 found)

None.

Important Issues (1 found)

1. A comment makes a false claim about buildFeed()'s behavior — same class of error as the E027 comment from last pass [feed_validation_test.go:320]. The comment reads:

   // buildFeed() deduplicates by design (one VehicleState per VehicleID).

   buildFeed() does not deduplicate — it iterates the input slice and emits one entity per element with no VehicleID-keyed dedup (handlers.go:181-219). The dedup actually happens upstream in the Tracker, which stores vehicles in a map[string]*VehicleState (tracker.go:24). Attributing a guarantee to the wrong function is precisely the trap we fixed in the E027 comment — a future maintainer reading this would believe buildFeed() is self-protecting against duplicate IDs when it isn't. It's a one-line fix; reword to credit the tracker, e.g.:

   // The Tracker dedups by VehicleID upstream (tracker.go), so buildFeed never
   // emits duplicate IDs in practice — inject a raw entity to exercise E052 directly.

Suggestions (3 found)

• The validateFeedCompliance helper still under-verifies itself [feed_validation_test.go:21-121]. You proved E050 and E052, but a mutation check shows the other enforcement branches can be neutralized to if false with every test still green. The high-value ones to add a rejection test for are the rules that guard buildFeed()'s own output construction — E038 (version), E048 (header timestamp), E049 (incrementality), E039 (is_deleted), and the header-level E050 branch (:116) — since a regression in buildFeed would slip past today's positive-only tests. The coordinate/bearing/timestamp branches (E026/E027/W001/W002/E001) are lower priority because production validate() rejects that input upstream, so they're only reachable via synthetic entities. Worth a follow-up, not a blocker.

• The E050 gap comment overstates what the test does [feed_validation_test.go:296-299]. "This test pins the known gap so it stays visible in CI" — but the body is solely t.Skip(...) with no assertions, so it pins nothing; it just emits a skip line. Soften to something like "Placeholder documenting the gap; skipped pending follow-up — no assertion is made."

• The helper docstring says it "exercises buildFeed()" [feed_validation_test.go:19-20], but the helper takes a *gtfs.FeedMessage and never calls buildFeed() — its callers do, and some pass hand-built or round-tripped feeds. Minor framing; reword to "callers typically build a feed via buildFeed() and pass the result here."

Strengths

• E012 logic is correct: headerTimestamp seeds to now and rises to the max entity timestamp, guaranteeing header.timestamp >= all entity timestamps even with a slightly-future entity, while staying at now for all-past feeds.
• The int64 → uint64 conversion is safe: the v.Timestamp <= 0 skip (handlers.go:183) runs before uint64(v.Timestamp), so no negative value can wrap into a huge unsigned number.
• NaN/Inf can't bypass validation: encoding/json rejects NaN literals and overflow-to-Inf at decode time, so a NaN bearing can never reach the < 0 || > 360 check via the ingest path.
• The bearing/speed handler tests are exemplary — real validate()/handler calls, true boundary values (-0.001, 360.001, exactly 0/360), explicit nil-is-valid cases, and both reject (400) and accept (201) paths.
• Test-quality cleanups landed correctly: time.Now()-relative timestamps replace stale hardcoded epochs, and assert.Eventually replaces a flaky time.Sleep.
• The protobuf round-trip test (TestFeedValidation_FeedSerializesCleanly) adds real marshal/unmarshal coverage.

Recommended Action

1. Fix the false buildFeed() dedup comment (feed_validation_test.go:320) — the only blocker, a one-line change.
2. Consider the helper rejection tests for E038/E048/E049/E039 + header E050 (follow-up is fine).
3. Soften the E050 "pins the gap" and helper-docstring wording.
4. Re-run review after the comment fix.

Verdict: Request Changes.