fix(ledger): remove implicit 7-day sliding window on volumes list#141
Conversation
BREAKING BEHAVIOR CHANGE: Previously, `fctl ledger volumes list` silently injected default time boundaries when the user did not provide them: - If `--end-time` was omitted, it defaulted to `time.Now()` - If `--start-time` was omitted, it defaulted to `endTime - 7 days` This meant that every volumes query was silently scoped to a 7-day sliding window, even when the user intended a Point-in-Time (PIT) query covering all historical data up to a given date. For example, running: fctl ledger volumes list --end-time "2025-06-12T12:00:00Z" Would send both `endTime=2025-06-12` AND `startTime=2025-06-05` to the ledger API. The ledger then applied: WHERE effective_date <= '2025-06-12' AND effective_date >= '2025-06-05' Any transaction older than 7 days before the requested PIT date was invisible, causing volumes to appear and disappear depending on the chosen date — which is incorrect for immutable ledger data. The ledger server correctly handles missing parameters: when no startTime is sent, it queries all moves from the beginning of time up to the endTime. The default injection was entirely client-side in fctl and never requested by the user. After this fix, only explicitly provided flags are sent to the API: - `--end-time` only → sends `endTime` only (true PIT query) - `--start-time` only → sends `startTime` only - Both flags → sends both (explicit time range) - Neither flag → sends neither (all data)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThis change removes automatic defaulting logic for datetime parameters in the volumes list command. Previously, null datetime values were automatically set to the current time and seven days prior; they are now passed through directly from the datetime parsing function without computed defaults. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
fguery
left a comment
There was a problem hiding this comment.
We should make sure that shows in the CHANGELOG, it's a big change
|
Maybe even update the docs? |
Summary
fctl ledger volumes listsilently injected default time boundaries, causing incorrect Point-in-Time (PIT) volume queries.When a user ran:
fctl ledger volumes list --end-time "2025-06-12T12:00:00Z"fctl sent both
endTime=2025-06-12andstartTime=2025-06-05(endTime minus 7 days) to the ledger API, even though the user never provided--start-time.This caused the ledger to generate:
Instead of the expected:
Any move older than 7 days before the requested PIT date was invisible, making volumes appear and disappear depending on the chosen date — which is incorrect for immutable ledger data.
Root cause
In
cmd/ledger/volumes/list.go, two default blocks injected time boundaries when the user did not provide them:The ledger server handles missing parameters correctly: when no
startTimeis sent, it queries all moves from the beginning of time up toendTime. The default injection was entirely client-side in fctl and never requested by the user.Behavior change
--end-timeonlyendTime+startTime(endTime - 7d)endTimeonly → true PIT query--start-timeonlystartTime+endTime(now)startTimeonlyendTime=now+startTime=now-7dVerification
Tested against a production stack. With the fix,
go run ./ ledger volumes list --end-time <date>returns the correct volumes covering all historical moves, while the installedfctlreturns empty results due to the 7-day window excluding the relevant transactions.Test plan
fctl ledger volumes list --end-time <date>→ should return all volumes up to that date (no lower bound)fctl ledger volumes list --start-time <date> --end-time <date>→ should return volumes in the explicit rangefctl ledger volumes listwithout time flags → should return all volumesfctl ledger volumes list --start-time <date>→ should return volumes from that date onward