fix(scheduler): datetime fidelity so FSRS learning steps stop collapsing to 'due today' - #58
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (33)
WalkthroughThis PR upgrades FSRS scheduling to preserve full UTC ChangesFSRS Datetime Fidelity and Step Persistence
F354 Module Docstring Fix and AIV Evidence
Sequence Diagram(s)sequenceDiagram
participant ReviewProcessor
participant FSRS_Scheduler
participant FlashcoreDB
ReviewProcessor->>FSRS_Scheduler: compute_next_state(card with step, last_review_date as datetime)
FSRS_Scheduler-->>ReviewProcessor: SchedulerOutput(next_due: datetime, step: int)
ReviewProcessor->>FlashcoreDB: add_review_and_update_card(review, new_card_state, new_step=step)
FlashcoreDB-->>ReviewProcessor: updated Card(next_due_date: datetime, step: int)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
flashcore/db/db_utils.py (1)
50-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the tuple-order docstring to include
step.
card_to_db_params_list()now emits a trailingcard.step, but the docstring still documentstag_countas the last field. That leaves the bulk-insert positional contract inaccurate.Proposed fix
- front_length, back_length, has_media, tag_count). + front_length, back_length, has_media, tag_count, step).Also applies to: 97-97
🤖 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 `@flashcore/db/db_utils.py` around lines 50 - 63, The positional tuple contract in card_to_db_params_list is outdated because the function now includes card.step but the docstring still ends with tag_count. Update the docstring in card_to_db_params_list to document the full tuple order with step as the final field, and make sure any related bulk-insert tuple documentation stays consistent with the emitted order.
🤖 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 `@flashcore/cli/review_ui.py`:
- Around line 125-127: The due-date calculation in review_ui’s card-update flow
mixes a datetime value with date.today(), which can raise a TypeError. Update
the logic around days_until_due_date to use matching types by converting
updated_card.next_due_date to a date or by comparing both values as UTC
datetimes before computing the difference. Keep the fix localized to the code
that builds the due-date message so the arithmetic and formatting stay
consistent.
In `@flashcore/db/database.py`:
- Line 533: `get_database_stats()` is still using a timezone-sensitive
`CURRENT_DATE + INTERVAL 1 DAY` cutoff for `due_count`, which can diverge from
the UTC normalization used by `get_due_card_count()` and `get_due_cards()`.
Update the `due_count` expression in `get_database_stats()` to use the same UTC
end-of-day boundary logic as the due queue helpers, so deck stats and due cards
agree around UTC midnight.
In `@flashcore/models.py`:
- Around line 93-100: Constrain Card.step to non-negative values so invalid
learning-step indices fail fast instead of flowing into fsrs_card.step. Update
the Card model in flashcore/models.py by adding a non-negative validation rule
to the step field (for example via Field constraints or a validator) while
preserving the existing Optional[int] behavior for None. Keep the check close to
the Card definition so any caller, including flashcore/scheduler.py, is
protected from negative values.
In `@flashcore/scheduler.py`:
- Around line 206-216: Normalize review_ts to UTC before computing elapsed_days
in the scheduler flow so date-based deltas are consistent. In the code path that
sets fsrs_card.last_review and later derives elapsed_days from review_ts.date(),
make sure the same _ensure_utc conversion is applied to review_ts first, then
use the normalized timestamp for the day difference calculation. Keep this
change near the existing card.last_review_date handling in scheduler logic so
FSRS receives the correct elapsed-day count.
In `@scripts/migrate_to_datetime_fidelity.py`:
- Around line 58-76: The migration checks in migrate_to_datetime_fidelity.py are
skipping any column whose type merely contains “TIMESTAMP,” which incorrectly
treats plain TIMESTAMP as already migrated. Update the guards around the
cards.next_due_date and reviews.next_due alterations to skip only when _col_type
returns TIMESTAMP WITH TIME ZONE (or an equivalent exact timestamptz match), so
plain TIMESTAMP columns still get altered to TIMESTAMP WITH TIME ZONE.
In `@tests/test_scheduler.py`:
- Around line 128-131: The follow-up review setup in the scheduler tests is
overriding FSRS’s returned due time by using replace() to pin reviews to fixed
clock times, which can make the test run before the card is actually due. Update
the affected test cases to use the scheduled timestamp from the prior result
directly (for example, derive the next review from initial_good_result.next_due
without forcing a specific hour/minute) in the test helpers and scenarios that
reference review2_ts and similar follow-up timestamps.
- Around line 527-530: The test helper add_review_side_effect now captures
new_step, but the assertion never verifies it, so a regression in
ReviewProcessor forwarding scheduler_output.step to add_review_and_update_card()
would go unnoticed. Update the test around add_review_side_effect/captured to
assert that the persisted new_step matches the expected scheduler output step,
alongside the existing review and new_card_state checks.
---
Nitpick comments:
In `@flashcore/db/db_utils.py`:
- Around line 50-63: The positional tuple contract in card_to_db_params_list is
outdated because the function now includes card.step but the docstring still
ends with tag_count. Update the docstring in card_to_db_params_list to document
the full tuple order with step as the final field, and make sure any related
bulk-insert tuple documentation stays consistent with the emitted order.
🪄 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
Run ID: ce1c2859-5391-49c6-ab1f-faaa4d640e02
📒 Files selected for processing (28)
.github/aiv-evidence/EVIDENCE_FLASHCORE_CLI_REVIEW_UI.md.github/aiv-evidence/EVIDENCE_TESTS_TEST_MODELS.PY.BUG_CATALOG.MD.md.github/aiv-evidence/EVIDENCE_TESTS_TEST_MODELS.md.github/aiv-packets/PACKET_flashcore_f354_ci.md.github/aiv-packets/PACKET_flashcore_f354_impl.md.github/aiv-packets/PACKET_flashcore_f354_tests.md.github/aiv-packets/VERIFICATION_PACKET_PR_FLASHCORE_F354.md.github/aiv-packets/evidence/flashcore-f354/MANIFEST.md.github/aiv-packets/evidence/flashcore-f354/baseline_red.txt.github/aiv-packets/evidence/flashcore-f354/class_c_negative_search.txt.github/aiv-packets/evidence/flashcore-f354/class_d_docstring_diff.txt.github/aiv-packets/evidence/flashcore-f354/head_green.txt.gitignoreflashcore/cli/review_ui.pyflashcore/db/database.pyflashcore/db/db_utils.pyflashcore/db/schema.pyflashcore/models.pyflashcore/review_processor.pyflashcore/scheduler.pyscripts/migrate_to_datetime_fidelity.pytests/test_db.pytests/test_models.pytests/test_models.py.bug-catalog.mdtests/test_review_logic_duplication.pytests/test_review_manager.pytests/test_review_processor.pytests/test_scheduler.py
| deck_name, | ||
| COUNT(*) AS card_count, | ||
| COUNT(CASE WHEN next_due_date <= CURRENT_DATE OR next_due_date IS NULL THEN 1 END) AS due_count | ||
| COUNT(CASE WHEN next_due_date < CURRENT_DATE + INTERVAL 1 DAY OR next_due_date IS NULL THEN 1 END) AS due_count |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In DuckDB 1.0.0, when TIMESTAMP WITH TIME ZONE values are compared against CURRENT_DATE or CURRENT_DATE + INTERVAL 1 DAY, is CURRENT_DATE evaluated in the connection/session timezone rather than UTC?
💡 Result:
In DuckDB 1.0.0, the CURRENT_DATE function returns the current date in UTC, not the connection or session timezone [1][2]. Because DuckDB 1.0.0 uses UTC for CURRENT_DATE, comparing it directly against a TIMESTAMP WITH TIME ZONE (TIMESTAMPTZ) value—which inherently represents an absolute point in time—will involve an implicit cast. When comparing a DATE to a TIMESTAMPTZ, DuckDB promotes the DATE to a TIMESTAMPTZ by treating it as midnight (00:00:00) in the configured connection time zone [3][4]. Consequently, even though CURRENT_DATE itself is UTC-based, the comparison logic often effectively introduces the connection's session timezone through this implicit promotion/casting process [3]. If your goal is to perform comparisons that strictly respect the session's local timezone for both the date component and the timestamp, you should be aware of this behavior, as it can lead to results that change depending on the current TIMEZONE setting [3]. Note that this behavior was identified as a PostgreSQL compatibility issue and has been updated in subsequent versions (post-1.0.0) to make CURRENT_DATE aware of the configured local timezone [5][6][7].
Citations:
- 1: https://duckdb.org/docs/1.0/sql/functions/date.html
- 2: https://database.guide/a-quick-look-at-duckdbs-current_date-function/
- 3: https://duckdb.org/docs/lts/guides/sql_features/timestamps.html
- 4: https://duckdb.org/docs/current/sql/data_types/timestamp.html
- 5: Postgres compatibility: CURRENT_DATE/CURRENT_TIME now return the current date/time in the users' time zone duckdb/duckdb-web#5150
- 6: current_date & current_time PostgreSQL incompatibility duckdb/duckdb#15069
- 7: Issue #15069: Postgres CURRENT_XXX Compatibility duckdb/duckdb#15125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate due-card helpers.
ast-grep outline flashcore/db/database.py --view expanded || true
printf '\n--- matches ---\n'
rg -n "get_due_card_count|get_due_cards|due_count|CURRENT_DATE|INTERVAL 1 DAY|end-of-day|UTC" flashcore/db/database.py flashcore -S || true
printf '\n--- relevant slice ---\n'
sed -n '430,590p' flashcore/db/database.pyRepository: ImmortalDemonGod/flashcore
Length of output: 12223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '370,430p' flashcore/db/database.pyRepository: ImmortalDemonGod/flashcore
Length of output: 2464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "next_due_date|TIMESTAMPTZ|timezone|UTC timestamp|full UTC datetime" flashcore/models.py flashcore/db/database.py flashcore/scheduler.py -SRepository: ImmortalDemonGod/flashcore
Length of output: 4325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '141,180p' flashcore/db/database.pyRepository: ImmortalDemonGod/flashcore
Length of output: 2187
🌐 Web query:
DuckDB 1.0.0 comparison TIMESTAMPTZ against CURRENT_DATE + INTERVAL 1 DAY implicit cast timezone boundary UTC end of day
💡 Result:
In DuckDB (version 1.0.0 and subsequent releases), comparing a TIMESTAMPTZ column against CURRENT_DATE + INTERVAL 1 DAY involves specific implicit casting and timezone behaviors that can significantly impact results [1][2]. Key mechanisms to consider: 1. DATE to TIMESTAMPTZ Casting: When comparing a TIMESTAMPTZ value with a DATE value (or an expression resulting in a DATE, such as CURRENT_DATE + INTERVAL), DuckDB implicitly casts the DATE to TIMESTAMPTZ [1][2]. This cast promotes the DATE to midnight (00:00:00) in the user's current configured timezone [1][2]. 2. Timezone Sensitivity: Because the promotion to midnight is timezone-dependent, the result of the comparison tstz_column <= CURRENT_DATE + INTERVAL 1 DAY will vary based on the session's current timezone setting (e.g., 'America/Los_Angeles' vs 'UTC' vs 'HST') [1][2]. Changing the timezone effectively shifts the "boundary" of what represents the start of that next day, potentially including or excluding specific timestamp instants near the midnight threshold [1][2]. 3. CURRENT_DATE behavior: Since recent updates (addressing PostgreSQL compatibility), CURRENT_DATE returns the current date in the session's local timezone rather than UTC [3][4]. 4. Practical Implications: - Boundary sensitivity: Comparisons using these expressions often result in "off-by-one" day errors or unexpected filtering results because the implicit cast assumes the local midnight boundary [1][2]. - Best Practice: To avoid these implicit timezone dependencies and boundary issues, it is recommended to perform temporal calculations using explicit conversions or by keeping data in a consistent timezone (preferably UTC) until display, or by using plain TIMESTAMP (without time zone) types for internal storage when the offset context is not required [1][2]. If you must compare them, verify the result in your target timezone using CAST(expression AS TIMESTAMPTZ) to observe the specific instant being generated [1]. For exact control, avoid relying on the implicit cast. Instead, explicitly cast the date to your desired timezone before performing the comparison [5][2].
Citations:
- 1: https://duckdb.org/docs/lts/guides/sql_features/timestamps
- 2: https://duckdb.org/docs/lts/guides/sql_features/timestamps.html
- 3: Postgres compatibility: CURRENT_DATE/CURRENT_TIME now return the current date/time in the users' time zone duckdb/duckdb-web#5150
- 4: current_date & current_time PostgreSQL incompatibility duckdb/duckdb#15069
- 5: Casting TIMESTAMPTZ to DATE should not be allowed duckdb/duckdb#3534
Use the same UTC end-of-day cutoff in get_database_stats()
get_due_card_count() and get_due_cards() already normalize bare dates to 23:59:59.999999 UTC, but due_count still uses CURRENT_DATE + INTERVAL 1 DAY. That boundary is timezone-dependent in DuckDB and can make deck stats disagree with the due queue around UTC midnight.
🤖 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 `@flashcore/db/database.py` at line 533, `get_database_stats()` is still using
a timezone-sensitive `CURRENT_DATE + INTERVAL 1 DAY` cutoff for `due_count`,
which can diverge from the UTC normalization used by `get_due_card_count()` and
`get_due_cards()`. Update the `due_count` expression in `get_database_stats()`
to use the same UTC end-of-day boundary logic as the due queue helpers, so deck
stats and due cards agree around UTC midnight.
| step: Optional[int] = Field( | ||
| default=None, | ||
| description=( | ||
| "FSRS learning/relearning step index (None in Review/New state)." | ||
| " Persisted so a card progresses through its learning steps" | ||
| " across reviews instead of restarting at step 0 each time." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Constrain Card.step to non-negative values.
flashcore/scheduler.py forwards this field straight into fsrs_card.step, so a negative value from the DB or any caller would silently corrupt learning-step progression instead of failing fast.
Suggested fix
step: Optional[int] = Field(
default=None,
+ ge=0,
description=(
"FSRS learning/relearning step index (None in Review/New state)."
" Persisted so a card progresses through its learning steps"
" across reviews instead of restarting at step 0 each time."
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| step: Optional[int] = Field( | |
| default=None, | |
| description=( | |
| "FSRS learning/relearning step index (None in Review/New state)." | |
| " Persisted so a card progresses through its learning steps" | |
| " across reviews instead of restarting at step 0 each time." | |
| ), | |
| ) | |
| step: Optional[int] = Field( | |
| default=None, | |
| ge=0, | |
| description=( | |
| "FSRS learning/relearning step index (None in Review/New state)." | |
| " Persisted so a card progresses through its learning steps" | |
| " across reviews instead of restarting at step 0 each time." | |
| ), | |
| ) |
🤖 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 `@flashcore/models.py` around lines 93 - 100, Constrain Card.step to
non-negative values so invalid learning-step indices fail fast instead of
flowing into fsrs_card.step. Update the Card model in flashcore/models.py by
adding a non-negative validation rule to the step field (for example via Field
constraints or a validator) while preserving the existing Optional[int] behavior
for None. Keep the check close to the Card definition so any caller, including
flashcore/scheduler.py, is protected from negative values.
| # Restore the learning/relearning step so the card progresses through | ||
| # its steps instead of restarting at step 0 every review. Without this, | ||
| # a Learning card never advances and can only escape via Easy. | ||
| if card.step is not None: | ||
| fsrs_card.step = card.step | ||
| if card.next_due_date: | ||
| fsrs_card.due = datetime.datetime.combine( | ||
| card.next_due_date, | ||
| datetime.time(0, 0, 0), | ||
| tzinfo=datetime.timezone.utc, | ||
| ) | ||
| # next_due_date is now a full UTC datetime — feed it to FSRS as-is | ||
| # (no date->midnight rounding) so sub-day step dues are honored. | ||
| fsrs_card.due = self._ensure_utc(card.next_due_date) | ||
| if card.last_review_date is not None: | ||
| fsrs_card.last_review = datetime.datetime.combine( | ||
| card.last_review_date, | ||
| datetime.time(0, 0, 0), | ||
| tzinfo=datetime.timezone.utc, | ||
| ) | ||
| fsrs_card.last_review = self._ensure_utc(card.last_review_date) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize review_ts before deriving elapsed_days.
card.last_review_date is converted to UTC here, but the later day diff still uses review_ts.date() before normalization. A non-UTC review_ts can therefore cross a date boundary and feed FSRS the wrong elapsed-day count.
Suggested fix
- # Manually calculate elapsed_days for the current review.
+ utc_review_ts = self._ensure_utc(review_ts)
+
+ # Manually calculate elapsed_days for the current review.
if hasattr(fsrs_card, "last_review") and fsrs_card.last_review:
elapsed_days = (
- review_ts.date() - fsrs_card.last_review.date()
+ utc_review_ts.date() - fsrs_card.last_review.date()
).days
else:
# For a new card, there are no elapsed days since a prior review.
elapsed_days = 0
# Now, apply the new review to the final state.
current_fsrs_rating = self._map_flashcore_rating_to_fsrs(new_rating)
- utc_review_ts = self._ensure_utc(review_ts)
try:🤖 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 `@flashcore/scheduler.py` around lines 206 - 216, Normalize review_ts to UTC
before computing elapsed_days in the scheduler flow so date-based deltas are
consistent. In the code path that sets fsrs_card.last_review and later derives
elapsed_days from review_ts.date(), make sure the same _ensure_utc conversion is
applied to review_ts first, then use the normalized timestamp for the day
difference calculation. Keep this change near the existing card.last_review_date
handling in scheduler logic so FSRS receives the correct elapsed-day count.
| if t and "TIMESTAMP" not in t.upper(): | ||
| con.execute( | ||
| "ALTER TABLE cards ALTER COLUMN next_due_date " | ||
| "TYPE TIMESTAMP WITH TIME ZONE" | ||
| ) | ||
| report["actions"].append(f"cards.next_due_date {t} -> TIMESTAMPTZ") | ||
| else: | ||
| report["actions"].append(f"cards.next_due_date already {t} (skip)") | ||
|
|
||
| # 2. reviews.next_due DATE -> TIMESTAMPTZ | ||
| t = _col_type(con, "reviews", "next_due") | ||
| if t and "TIMESTAMP" not in t.upper(): | ||
| con.execute( | ||
| "ALTER TABLE reviews ALTER COLUMN next_due " | ||
| "TYPE TIMESTAMP WITH TIME ZONE" | ||
| ) | ||
| report["actions"].append(f"reviews.next_due {t} -> TIMESTAMPTZ") | ||
| else: | ||
| report["actions"].append(f"reviews.next_due already {t} (skip)") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In DuckDB 1.0.0, what exact values can information_schema.columns.data_type return for TIMESTAMP and TIMESTAMP WITH TIME ZONE columns?
💡 Result:
In DuckDB 1.0.0, the information_schema.columns.data_type column returns the following values for timestamp-related columns: * For TIMESTAMP columns, it returns 'TIMESTAMP'. * For TIMESTAMP WITH TIME ZONE columns, it returns 'TIMESTAMP WITH TIME ZONE'. These values correspond to the primary type names used in DuckDB's SQL dialect [1][2]. While DuckDB provides various timestamp aliases (e.g., DATETIME for TIMESTAMP or TIMESTAMPTZ for TIMESTAMP WITH TIME ZONE), the information_schema typically reports the canonical type name defined in the system catalog [2][3]. Note that DuckDB also supports other specific timestamp variants (such as TIMESTAMP_MS, TIMESTAMP_NS, and TIMESTAMP_S) which may be returned as their respective internal type names if used [4][1].
Citations:
- 1: https://duckdb.org/docs/current/sql/data%5Ftypes/timestamp
- 2: https://duckdb.org/docs/current/sql/data_types/overview
- 3: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/lts/sql/data_types/timestamp.md
- 4: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/1.0/sql/data_types/timestamp.md
Only skip columns that are already TIMESTAMP WITH TIME ZONE.
The current guard treats any type containing "TIMESTAMP" as already migrated, so a plain TIMESTAMP column will be skipped and never upgraded to TIMESTAMP WITH TIME ZONE. That leaves the schema short of the intended target.
🤖 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 `@scripts/migrate_to_datetime_fidelity.py` around lines 58 - 76, The migration
checks in migrate_to_datetime_fidelity.py are skipping any column whose type
merely contains “TIMESTAMP,” which incorrectly treats plain TIMESTAMP as already
migrated. Update the guards around the cards.next_due_date and reviews.next_due
alterations to skip only when _col_type returns TIMESTAMP WITH TIME ZONE (or an
equivalent exact timestamptz match), so plain TIMESTAMP columns still get
altered to TIMESTAMP WITH TIME ZONE.
| def add_review_side_effect(*, review, new_card_state, new_step=None): | ||
| captured["review"] = review | ||
| captured["new_card_state"] = new_card_state | ||
| captured["new_step"] = new_step |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that new_step is forwarded to persistence.
This mock now captures new_step, but the test never checks it. ReviewProcessor could stop passing scheduler_output.step to add_review_and_update_card() and this regression would still pass.
Suggested fix
assert captured["review"].card_uuid == card.uuid
assert captured["review"].rating == 3
assert captured["review"].next_due == scheduler_output.next_due
assert captured["new_card_state"] == scheduler_output.state
+ assert captured["new_step"] == scheduler_output.step🤖 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 `@tests/test_scheduler.py` around lines 527 - 530, The test helper
add_review_side_effect now captures new_step, but the assertion never verifies
it, so a regression in ReviewProcessor forwarding scheduler_output.step to
add_review_and_update_card() would go unnoticed. Update the test around
add_review_side_effect/captured to assert that the persisted new_step matches
the expected scheduler output step, alongside the existing review and
new_card_state checks.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #58 +/- ##
==========================================
- Coverage 93.24% 93.23% -0.02%
==========================================
Files 24 24
Lines 2133 2143 +10
==========================================
+ Hits 1989 1998 +9
- Misses 144 145 +1
🚀 New features to boost your workflow:
|
5b1a065 to
734de6f
Compare
734de6f to
f49ed7e
Compare
1 participant
AIV Verification Packet (v2.2)
Identification
dfd5d5f,0440ed3,47f0ba7,5d96cc4,5f034db,103312d,85959f3,e8df5fb,966032e,fd41993,2d28a38,d7a5cdf,77929c7,f97a3ca,661a3f9,b826924b826924fb1ae5aClassification
Claims
Evidence References
dfd5d5f0440ed347f0ba75d96cc45f034db103312d85959f3e8df5fb966032efd419932d28a38d7a5cdf77929c7f97a3ca661a3f9b826924Class E (Intent Alignment)
Class B (Referential Evidence)
Scope Inventory (from 69 file references across evidence files)
flashcore/models.py#L11flashcore/models.py#L61flashcore/models.py#L64-L66flashcore/models.py#L69flashcore/models.py#L71-L75flashcore/models.py#L88-L95flashcore/models.py#L252flashcore/models.py#L254-L258flashcore/scheduler.py#L41-L43flashcore/scheduler.py#L48flashcore/scheduler.py#L208-L212flashcore/scheduler.py#L214-L216flashcore/scheduler.py#L218flashcore/scheduler.py#L268-L270flashcore/scheduler.py#L277flashcore/review_processor.py#L104-L106flashcore/review_processor.py#L132-L133flashcore/review_processor.py#L135-L137flashcore/review_processor.py#L140flashcore/db/schema.py#L17flashcore/db/schema.py#L21flashcore/db/schema.py#L46flashcore/db/database.py#L145-L146flashcore/db/database.py#L176-L180flashcore/db/database.py#L400-L407flashcore/db/database.py#L416flashcore/db/database.py#L453-L462flashcore/db/database.py#L467flashcore/db/database.py#L533flashcore/db/database.py#L704flashcore/db/database.py#L717-L718flashcore/db/database.py#L726flashcore/db/database.py#L733-L736flashcore/db/database.py#L746flashcore/db/database.py#L758flashcore/db/database.py#L783-L786flashcore/db/database.py#L807flashcore/db/db_utils.py#L97flashcore/cli/review_ui.py#L125-L127flashcore/cli/_review_all_logic.py#L88scripts/migrate_to_datetime_fidelity.py#L1-L151tests/test_scheduler.py#L53-L61tests/test_scheduler.py#L128-L131tests/test_scheduler.py#L215tests/test_scheduler.py#L241-L242tests/test_scheduler.py#L254tests/test_scheduler.py#L369-L370tests/test_scheduler.py#L395-L396tests/test_scheduler.py#L505tests/test_scheduler.py#L510tests/test_scheduler.py#L527tests/test_scheduler.py#L530tests/test_scheduler.py#L613-L614tests/test_scheduler.py#L820-L901tests/test_db.py#L1009-L1010tests/test_db.py#L1050-L1051tests/test_review_manager.py#L48tests/test_review_manager.py#L52tests/test_review_manager.py#L563tests/test_review_processor.py#L57tests/test_review_processor.py#L62tests/test_review_logic_duplication.py#L68tests/cli/test_review_ui.py#L5tests/cli/test_review_ui.py#L55tests/cli/test_review_ui.py#L98-L100tests/cli/test_review_ui.py#L341tests/cli/test_review_ui.py#L382tests/cli/test_review_all_logic.py#L138-L140tests/cli/test_review_all_logic.py#L538-L540Class F (Provenance)
The defect was real, observed in production before the fix.
Production symptom (2026-06-29): on the live deck, 21
Learning-state cards all hadnext_due_datepinned to the current calendar date (00:00 = already overdue) and were re-drilled repeatedly within a single session; onlyEasy-rated cards ever advanced to a forward date. Query evidence:reviewsfor 2026-06-29 showed everyAgain/Hard/Goodrating producingscheduled_days_interval = 0andnext_due = today, whileEasyproduced multi-day intervals.Root cause, two parts (both fixed here):
FSRS_Scheduler.compute_next_statereturnednext_due = updated_fsrs_card.due.date()— truncating py-fsrs's datetimedue(1m/10m learning steps) to a bare date, collapsing every sub-day step to "due today".FSRSCard()was rebuilt each review without restoringcard.step, andCardhad nostepfield, so a Learning card restarted at step 0 every review and could only leave Learning viaEasy.RED -> GREEN regressions (fail on the pre-fix engine, pass after —
tests/test_scheduler.py):test_learning_step_due_is_future_subday— asserts a learning step'snext_dueis a future, same-day, sub-day datetime (RED: pre-fix returnedreview_ts.date()).test_step_persistence_lets_learning_card_graduate— asserts repeatedGoodgraduates the card oncestepis persisted (RED: pre-fix looped on "due today" forever).End-to-end on a copy of the real DB (post-migration): a
Learningcard moved from2026-06-28 00:00(overdue) ->now + 0:10:00(real 10-minute step,step0->1) -> graduated to a 6-day interval on the nextGood.Upstream confirmation: py-fsrs 6.3.x —
Card.dueis adatetime;learning_steps = (1m, 10m). The minute-level scheduling is correct upstream; the truncation was flashcore-side.Verification Methodology
Zero-Touch Mandate: Verifier inspects artifacts only.
Evidence was collected by
aiv commitduring the change lifecycle.Packet generated by
aiv close.Known Limitations
Use
git show <sha>:.github/aiv-evidence/<file>to retrieve.Summary
Change 'fsrs-datetime-fidelity': 16 commit(s) across 16 file(s).