Fix duration rounding at unit boundaries

[zzbcode]

What changed

• normalize duration values before choosing minute and hour buckets
• carry rounded 60.0s, 1m 60s, and 59m 60s values into the next unit
• add regression coverage for second-to-minute and minute-to-hour boundaries

Why

formatDuration previously chose a unit bucket before rounding its remainder. Values near a boundary could therefore render impossible strings such as 60.0s, 1m 60s, or 59m 60s.

The updated implementation rounds at the precision used by each bucket, preserving normal values such as 59.6s while carrying only values that cross a display boundary.

Validation

• 15 direct assertions against the TypeScript module, including all existing cases and the new boundary cases
• git diff --check
• Full Bun/Vitest validation is expected in CI; local Bun setup was unavailable during development

Fixes #521.

This is my first contribution to Failproof AI. If the project maintains a contributors list or recognition process beyond GitHub's merged-PR attribution, I would be grateful to be included once this is merged.

Summary by CodeRabbit

• Bug Fixes

  • Improved duration formatting when rounding seconds reaches the next minute.
  • Improved duration formatting when rounding minutes reaches the next hour.
  • Ensured displayed time values consistently roll over between seconds, minutes, and hours.

• Tests

  • Added coverage for duration rounding and unit-boundary rollover scenarios.

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e8f1cb2a-475f-4e9d-ab87-08f91a3bf5ab

📥 Commits

Reviewing files that changed from the base of the PR and between 446484b and df65047.

📒 Files selected for processing (2)

• __tests__/lib/format-duration.test.ts
• lib/format-duration.ts

 _______________________________________________________________________
< You could simplify this. Not because I said so - because reality did. >
 -----------------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages.

---

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.}

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

putComment timed out

[hermes-exosphere]

Automated code review started - full review. Results will be posted here.

[hermes-exosphere]

⏳ [Hermes Review — Progress Update]

Phase 0-2 complete. Findings so far:

• 2 files changed, 15 insertions, 3 deletions
• 13 unit tests: ✅ ALL PASS
• TypeScript: ✅ no errors
• ESLint on changed files: ✅ clean
• Logic analysis complete — fix is correct and addresses all 3 bug patterns from formatDuration emits invalid strings at rounding boundaries ("60.0s", "1m 60s", "59m 60s") #521

Running full test suite and Docker smoke test now...

[hermes-exosphere]

✅ [Hermes Review - Build and Test Complete]

Results:

• Build: pass (TypeScript: no errors, ESLint on changed files: clean)
• Unit tests: 2065/2066 pass - the 1 failure is pre-existing (Pi integration > writeHookEntries path mismatch in a temp directory, unrelated to this change)
• format-duration tests: 13/13 pass (including all new boundary tests)

Logic verification:

• Bug formatDuration emits invalid strings at rounding boundaries ("60.0s", "1m 60s", "59m 60s") #521 cases verified fixed: 59999ms to "1m 0s" (was "60.0s"), 119600ms to "2m 0s" (was "1m 60s"), 3599600ms to "1h 0m" (was "59m 60s")
• Rounding boundary at 59.95s is correct: values at or above 59.95s round up to 60.0 and properly carry into minutes
• All existing test cases continue to pass with no regressions
• No floating-point issues at large values (tested up to 31,536,000,000ms)

Full review summary posting now...

[hermes-exosphere]

🔍 Automated Code Review

📋 Executive Summary

This PR fixes a rounding bug in formatDuration where values near unit boundaries could produce invalid display strings like "60.0s", "1m 60s", or "59m 60s". The fix is minimal (8 lines changed), well-targeted, and adds proper test coverage. The implementation rounds before bucket selection instead of after, correctly carrying boundary values into the next unit. Code quality is high with no new issues introduced.

---

📊 Change Architecture

graph TD
    A["formatDuration(ms)"] -->|"ms / 1000"| B["seconds (raw)"]
    B -->|"Math.round(s*10)/10"| C["roundedTenths"]
    C -->|"< 60?"| D["SECONDS BUCKET: toFixed(1)"]
    C -->|">= 60"| E["roundedSeconds = Math.round(s)"]
    E -->|"floor(rs/60)"| F["totalMinutes"]
    F -->|">= 60?"| G["HOURS BUCKET: h/m output"]
    F -->|"< 60"| H["MINUTES BUCKET: m/s output"]
    style C fill:#90EE90
    style E fill:#90EE90
    style D fill:#87CEEB
    style H fill:#87CEEB

Loading

Legend: 🟢 New (or changed) | 🔵 Modified (existing path)

---

🔴 Breaking Changes

✅ No breaking changes detected. The function signature is unchanged, the return type is unchanged, and all existing test assertions continue to pass without modification. The output changes ONLY for values near boundaries that previously produced invalid strings (e.g., "60.0s" → "1m 0s"), which is the intended fix.

---

⚠️ Issues Found

1. 🔵 Informational — lib/format-duration.ts:20 — Rounding strategy changed from toFixed(1) (banker's rounding) to Math.round(seconds * 10) / 10 (standard rounding). This means values like 59.05s now produce "59.1s" instead of "59.0s". This is intentional and documented, but worth noting for anyone who expected the old truncation behavior. The change is consistent: all rounding now uses Math.round for uniformity.

---

🔬 Logical / Bug Analysis

Bug #1: "60.0s" — Old code checked seconds < 60 using the raw value (e.g., 59.999), then formatted with toFixed(1) which rounded to "60.0". New code pre-rounds to tenths: Math.round(59.999 * 10) / 10 = 60.0, which is NOT < 60, so it correctly enters the minutes branch. ✅ Fixed.

Bug #2: "1m 60s" — Old code computed totalMinutes = floor(119.6 / 60) = 1, then (119.6 % 60).toFixed(0) = "60". New code rounds seconds first: Math.round(119.6) = 120, then floor(120 / 60) = 2 and 120 % 60 = 0 → "2m 0s". ✅ Fixed.

Bug #3: "59m 60s" — Same root cause as Bug #2 but at the minute level. Old floor(3599.6 / 60) = 59, (3599.6 % 60).toFixed(0) = "60". New Math.round(3599.6) = 3600, floor(3600 / 60) = 60, then floor(60 / 60) = 1, 60 % 60 = 0 → "1h 0m". ✅ Fixed.

Rounding consistency: The decision to change from toFixed(1) (banker's rounding) to Math.round means values exactly at the 0.5 midpoint round up instead of to-nearest-even. For example, 59050ms (59.05s) now produces "59.1s" instead of "59.0s". This is a deliberate design choice and is applied uniformly across all units.

No other issues found: No race conditions, no resource leaks, no missing null checks, no incorrect async patterns, no copy-paste errors.

---

🧪 Evidence — Build & Test Results

Build Output (TypeScript + ESLint)

TypeScript (tsc --noEmit): exit 0, no errors
ESLint on changed files: exit 0, no warnings

Test Results (format-duration only)

 ✓ __tests__/lib/format-duration.test.ts (13 tests) 15ms
   Tests  13 passed (13)

All existing cases pass plus 4 new boundary tests:

• 59600ms → "59.6s" (stays in seconds, near boundary)
• 59999ms → "1m 0s" (crosses boundary correctly)
• 119600ms → "2m 0s" (crosses with carry)
• 3599600ms → "1h 0m" (crosses to hours correctly)

Full Test Suite

Test Files  120 passed | 1 failed (121)
     Tests  2065 passed | 1 failed (2066)

The 1 failure (Pi integration > writeHookEntries: expected path to contain 'failproofai') is pre-existing and unrelated to this change — caused by temp directory naming in the test runner.

---

🔗 Issue Linkage

This PR addresses #521 — "formatDuration emits invalid strings at rounding boundaries". The three specific bug cases from the issue description are all verified fixed. Changes align exactly with the issue's expected behavior.

---

👥 Human Review Feedback

No human review comments on this PR. The only prior comments are from bots (coderabbitai, hermes-exosphere).

---

💡 Suggestions

1. Documentation — Consider adding a JSDoc comment to formatDuration documenting that rounding uses Math.round (standard rounding) and values within 0.05s of the minute boundary will promote to the next unit. This would help future contributors understand the expected behavior.

2. Test coverage — The current tests are thorough for the fixed bugs, but one additional edge case could be added:

   • expect(formatDuration(59050)).toBe("59.1s") — explicitly documents the rounding direction for the 0.5 midpoint

3. Negative values — The function returns "-100ms" for negative input. While durations are typically non-negative, consider whether a guard clause (Math.max(0, ms)) would be appropriate, or if negative values should be supported (they work fine as-is, just look odd).

---

🏆 Verdict

VERDICT: APPROVED

---

Automated code review by Hermes Agent · 2026-07-16 18:02 UTC

[hermes-exosphere]

Automated review: Approved. ✅

All 13 tests pass (including new boundary cases). TypeScript and ESLint are clean. The fix correctly addresses all three bug patterns from #521. No regressions detected. Full review summary posted above.

[hermes-exosphere]

Automated review: Approved. ✅

[hermes-exosphere]

Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb

[hermes-exosphere]

Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai

[hermes-exosphere]

Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai

[chhhee10]

Thanks @zzbcode — logic's correct, I ran it across the boundary spread and it lines up with the others exactly.

Snag is #529 is on the same issue and got there first with a wider boundary suite, so I'll probably go with that one rather than double-merge the same fix. Sorry for the duplicated effort — #534 is the meatier one of yours and I'm going through it properly now.