feat(timeentry): reject single-bound PATCH inversion at the controller layer (#130 follow-up) (#156)

#130 added a zod `.refine()` on createTimeEntryBody and
updateTimeEntryBody that rejects teEndedAt < teStartedAt when both
bounds are in the request body. The PR called out the
single-bound PATCH case as intentionally NOT covered there: a
PATCH that supplies only teEndedAt (or only teStartedAt) merges
against the row's existing value, which the schema layer can't
see without a DB read. Said:

> The single-bound PATCH case (only teEndedAt, validated against
> the row's existing teStartedAt) can't be enforced at the schema
> layer without a DB read. Leave that on the controller layer
> for a follow-up.

This is that follow-up. After the controller computes
`mergedStart` / `mergedEnd` from the body + existing row, check
whether the merged range is inverted; reject with 400 instead of
silently dropping `teMinutes` to null (the previous behaviour,
which the PR review confirmed had real-world bookkeeping cost:
"clocked out before clocked in" rows look correct in their two
timestamp columns but have an empty duration).

Factored the check into a small `isInvertedRange(startedAt,
endedAt)` helper, exposed via `_internals` for unit testing.
Equality (zero-minute "Due on Receipt"-style entries) stays
allowed. Unparseable input returns false from the helper because
computeMinutes' own NaN guard already null-tags those rows;
flagging them at this layer too would be double-counting a
problem we'd 400 on elsewhere.

Four new unit tests on the helper cover the four classes:
missing-bound (false), happy-path including equality (false),
inverted (true), unparseable (false).

Co-authored-by: Aaron K. Clark <akclark@thenetwerk.net>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 3 people

app/controllers/timeentrycontroller.js

@@ -30,6 +30,29 @@ function computeMinutes(startedAt, endedAt) {
     return Math.round((end - start) / 60000);
 }
 
+/**
+ * Return true when both bounds are non-empty AND parse cleanly AND
+ * endedAt is strictly before startedAt. Used by the PATCH handler to
+ * reject single-bound updates whose merged interval would invert.
+ *
+ * The schema-layer refinement on updateTimeEntryBody (#130) catches
+ * the both-bounds-in-body case; this helper covers the half the
+ * schema can't see — a PATCH that supplies only one of the two
+ * bounds and merges against the row's existing value.
+ *
+ * Equality is NOT inverted (zero-minute entries are legitimate).
+ * Unparseable input is NOT inverted (computeMinutes will surface
+ * the null-result via its own NaN guard; flagging it here would
+ * be a false positive on garbage we'd 400 elsewhere anyway).
+ */
+function isInvertedRange(startedAt, endedAt) {
+    if (!startedAt || !endedAt) return false;
+    const start = new Date(startedAt).getTime();
+    const end = new Date(endedAt).getTime();
+    if (!Number.isFinite(start) || !Number.isFinite(end)) return false;
+    return end < start;
+}
+
 /**
  * POST /v1/timeentry
  *
@@ -236,10 +259,25 @@ exports.update = async (req, res) => {
     }
     // Recompute minutes if either bound changed.
     if (updates.teStartedAt !== undefined || updates.teEndedAt !== undefined) {
-        updates.teMinutes = computeMinutes(
-            updates.teStartedAt !== undefined ? updates.teStartedAt : entry.teStartedAt,
-            updates.teEndedAt !== undefined ? updates.teEndedAt : entry.teEndedAt,
-        );
+        const mergedStart = updates.teStartedAt !== undefined
+            ? updates.teStartedAt : entry.teStartedAt;
+        const mergedEnd = updates.teEndedAt !== undefined
+            ? updates.teEndedAt : entry.teEndedAt;
+        // Inverted-range guard for the single-bound PATCH case. The
+        // schema-layer refinement in updateTimeEntryBody (#130) only
+        // sees fields present in the request body, so a PATCH that
+        // supplies only one bound can't be validated there — the
+        // row's existing value lives in the DB. Reject merged
+        // end < start at 400 instead of silently dropping `teMinutes`
+        // to null and storing a row whose clocked-out-before-clocked-
+        // in timestamps look correct but whose duration column is
+        // blank.
+        if (isInvertedRange(mergedStart, mergedEnd)) {
+            return res.status(400).json({
+                message: 'teEndedAt must be at or after teStartedAt.',
+            });
+        }
+        updates.teMinutes = computeMinutes(mergedStart, mergedEnd);
     }
     try {
         await entry.update(updates);
@@ -414,4 +452,4 @@ exports.exportCsv = async (req, res) => {
 };
 
 // Exposed for unit testing.
-exports._internals = { computeMinutes, IsMaster, GetCompanyId };
+exports._internals = { computeMinutes, isInvertedRange, IsMaster, GetCompanyId };

tests/api/timeentry.test.js

@@ -187,6 +187,54 @@ describe('PATCH /v1/timeentry/:id auth contract', () => {
     });
 });
 
+describe('isInvertedRange helper', () => {
+    // The PATCH /v1/timeentry/:id handler uses this to reject the
+    // single-bound update case where the schema-layer refinement
+    // can't see the row's existing other bound. Returning true means
+    // "reject with 400"; false means "let it through".
+    //
+    // Unit-tested in isolation rather than via supertest because the
+    // full PATCH path requires auth + DB mocks that don't compose
+    // cleanly with vitest's per-file vi.mock model for this codebase.
+    // The controller is a 1-line `if (isInvertedRange(a, b)) return
+    // res.status(400)...`, so a unit test on the helper is the
+    // tightest coverage.
+    let isInvertedRange;
+    beforeAll(async () => {
+        const ctrl = await import('../../app/controllers/timeentrycontroller.js');
+        isInvertedRange = ctrl._internals.isInvertedRange;
+    });
+
+    test('false when either bound is missing (open-ended entry, legitimate)', () => {
+        expect(isInvertedRange(null, '2026-05-15T10:00:00Z')).toBe(false);
+        expect(isInvertedRange('2026-05-15T10:00:00Z', null)).toBe(false);
+        expect(isInvertedRange(null, null)).toBe(false);
+        expect(isInvertedRange(undefined, undefined)).toBe(false);
+    });
+
+    test('false when ended >= started (the happy paths)', () => {
+        expect(isInvertedRange('2026-05-15T09:00:00Z', '2026-05-15T10:00:00Z')).toBe(false);
+        // Equality: zero-minute entry is legitimate.
+        expect(isInvertedRange('2026-05-15T09:00:00Z', '2026-05-15T09:00:00Z')).toBe(false);
+    });
+
+    test('true when ended < started (the bug we reject at 400)', () => {
+        expect(isInvertedRange('2026-05-15T10:00:00Z', '2026-05-15T09:00:00Z')).toBe(true);
+        // Even one millisecond difference flips the bit.
+        expect(isInvertedRange('2026-05-15T10:00:00.001Z', '2026-05-15T10:00:00.000Z')).toBe(true);
+    });
+
+    test('false on unparseable input (computeMinutes guards null elsewhere)', () => {
+        // The helper deliberately does NOT 400 on garbage strings — those
+        // get null teMinutes via computeMinutes' NaN guard. Treating them
+        // as "inverted" here would be a false positive flagging input
+        // that's already otherwise-broken.
+        expect(isInvertedRange('not-a-date', '2026-05-15T10:00:00Z')).toBe(false);
+        expect(isInvertedRange('2026-05-15T10:00:00Z', 'also-not-a-date')).toBe(false);
+        expect(isInvertedRange('garbage', 'garbage')).toBe(false);
+    });
+});
+
 describe('DELETE /v1/timeentry/:id auth contract', () => {
     test('returns 403 when authKey header is missing', async () => {
         const res = await request(app).delete('/v1/timeentry/1');