* fix(memory): consolidated facts inherit expected_valid_days from sources
Consolidation (#3996) and per-fact expected_valid_days (#4143) were both
authored by the same contributor but never connected: the consolidated
new_fact carried the newest source's createdAt but no expected_valid_days,
so _effective_fact_staleness_age fell back to the global staleness_age_days.
A merge of several 200-day-old stable facts (each evd=3650) would land with
no evd, read as a 90-day window, and re-enter the staleness candidate set on
the very next cycle - the merge discarded the lifetime signal of the
underlying information and contradicted consolidation's premise (these are
stable, related facts worth synthesising).
Fix: the merged fact inherits expected_valid_days set so it is re-reviewed at
the EARLIEST source review deadline (min(createdAt + expected_valid_days)
across sources, relative to the merged fact's createdAt = the newest source's).
A merge combines details from every source, so a volatile sub-detail (evd=7)
must not inherit a stable source's 3650-day window and escape staleness review
for years - staleness KEEP/REMOVE is the only path that re-validates a merged
fact, so biasing toward the soonest deadline keeps uncertain merges re-checked
sooner. A source already past its deadline yields a minimal positive window
(review next cycle) rather than the global fallback, which would defer an
overdue review. Capped at the creation-time staleness_max_lifetime_multiplier
like any new fact. Omitted when no source carries a valid evd (legacy facts
fall back to the global age at read time, matching pre-feature behaviour).
DRY: extract _read_expected_valid_days(fact) -> int | None, the shared type
rule (int/float, reject bool, coerce to int BEFORE the > 0 guard) previously
inlined in four places - _normalize_memory_update_fact,
_effective_fact_staleness_age, the newFact creation cap, and consolidation
inheritance. All four call the single helper. Coercing before the guard
matters for values in (0, 1): 0.5 passes a raw > 0 check but truncates to 0,
which would violate the helper's "positive int or None" contract; the order
now matches the original _normalize_memory_update_fact rule.
No prompt/schema change: consolidation's prompt does not surface source evd to
the LLM, so asking the model to assign a merged lifetime would be guessing
without signal. Source inheritance is deterministic and always available.
Tests: consolidation evd cases now use time-stable createdAt (relative to now
via a _days_ago helper) covering - earliest-deadline selection, creation-cap
clamp, omit when no source evd, volatile source governs the deadline (and
re-enters staleness next cycle), overdue source clamps to a minimal window,
float coercion. Plus TestReadExpectedValidDays / TestEffectiveFactStalenessAge
regression cases for the (0, 1) coercion-order fix.
* fix(memory): reject non-finite expected_valid_days before int coercion
The shared _read_expected_valid_days helper (introduced when consolidating the
evd type rule across four call sites) coerces with int(raw) before the > 0
guard - reversing the original _normalize_memory_update_fact order so that a
fractional 0.5 does not leak as 0. But int(raw) raises for non-finite floats:
int(nan) raises ValueError and int(inf)/int(-inf) raise OverflowError. Python's
JSON decoder accepts NaN / Infinity as floats by default, so a single malformed
expected_valid_days in a hand-edited memory.json would abort staleness selection
or consolidation instead of falling back to the global lifetime.
On main, _effective_fact_staleness_age checked raw > 0 first, so NaN fell back
safely (nan > 0 is false) - but inf did NOT (inf > 0 is true, so main also
crashed on inf). This helper is now the persisted-fact read path for both
staleness and consolidation, so the regression (and the pre-existing inf crash)
must be closed here.
Fix: require math.isfinite(float(raw)) before the int() coercion, then keep
the existing positivity check and fallback. NaN / +/-inf all return None, so
callers fall back to the global staleness_age_days. Normal int/float values
(including large ones) are unaffected - isfinite is a no-op for them.
Tests:
- TestReadExpectedValidDays.test_rejects_non_finite_values - NaN, inf, -inf
return None (not raise).
- TestEffectiveFactStalenessAge.test_falls_back_for_non_finite_values - the
persisted-fact read path returns the global age for each, no raise.
- test_consolidation_with_non_finite_source_evd_does_not_raise - end-to-end:
a NaN-evd source merged with a stable source does not abort consolidation;
the NaN source's effective lifetime falls back to the global 90 and its
deadline participates in the earliest-deadline computation.
* test(memory): hoist _select_stale_candidates import + tidy deadline docstring
Two review nits from the latest pass:
- `_select_stale_candidates` was imported inline inside three test methods;
hoisted to the module-level import block so the dependency is declared once.
- `test_consolidated_evd_volatile_source_with_equal_created_at_future_deadline`
had an abandoned calculation in its comment ("3 + 7 = 10 ... minus 3 already
elapsed = 7? No:") that could mislead future readers into thinking
elapsed-since-creation factors into the inherited window. Collapsed to a
single clear line stating the window is relative to the merged createdAt,
regardless of the source's current age.
* fix(memory): reject huge-int expected_valid_days above timedelta.max.days
_read_expected_valid_days routed every numeric value through float(raw) for the
math.isfinite guard, but Python's JSON decoder parses an integer literal with
no decimal point as an arbitrary-precision int (unlike 1e400, which decodes to
float inf). So a hand-edited memory.json carrying "expected_valid_days": 10**400
raised OverflowError in float(raw) before math.isfinite was ever called -
exactly the malformed-field-aborts-everything scenario the helper's docstring
claims to prevent.
The earlier non-finite fix only closed the float cases (NaN / +/-inf / 1e400).
A huge int below the float limit but above timedelta.max.days (e.g. 10**12)
would pass the helper and raise OverflowError downstream in
timedelta(days=evd) during staleness selection or consolidation - the same
crash fancyboi999 flagged for extend_by_days, just reached via a stored evd.
Fix: branch on type so an int never passes through float() (matching the
reviewer's suggestion), AND cap the returned int at timedelta.max.days
(999999999) so the downstream timedelta(days=evd) call cannot overflow either.
The float branch keeps the isfinite + int() coercion. Both branches share the
0 < evd <= timedelta.max.days positivity/range check.
Normal values are unaffected - any legitimate expected_valid_days is far below
the cap (the config ceiling staleness_max_extension_days tops out at 36500).
Tests (all three layers):
- TestReadExpectedValidDays.test_rejects_huge_int_above_timedelta_max - 10**400,
10**12, 10**9, timedelta.max.days+1 return None; timedelta.max.days itself
is accepted.
- TestEffectiveFactStalenessAge.test_falls_back_for_huge_int_above_timedelta_max
- the persisted-fact read path returns the global age, no raise.
- test_consolidation_with_huge_int_source_evd_does_not_raise[1e400|1e12|1e9] -
parametrized end-to-end: a huge-int-evd source merged with a stable source
does not abort consolidation; the bad source falls back to the global 90.
* fix(memory): guard datetime arithmetic, not just timedelta construction
The huge-int fix capped _read_expected_valid_days at timedelta.max.days, but
that only proves timedelta(days=evd) can be constructed - adding it to a real
fact timestamp still overflows datetime.max. @fancyboi999 reproduced it: a
source with expected_valid_days=timedelta.max.days raises
"OverflowError: date value out of range" at dt + timedelta(...) in the new
consolidation deadline calculation. capping at timedelta.max.days was another
patch chasing the next overflow boundary, not a real close.
Root cause: the helper was doing datetime-range validation, but the safe bound
depends on the datetime the evd is added to, not on the evd alone. So the
responsibility moves to the arithmetic site, with try/except as the terminal
guard - no concrete upper bound to be wrong about.
Changes:
- _read_expected_valid_days returns any positive int (huge ints included, not
routed through float). Its job is type/positivity validation only.
- New _safe_add_days(dt, days) -> datetime | None wraps dt + timedelta(days),
returning None on OverflowError/ValueError. This is the terminal guard -
there is no further boundary to overflow because try/except catches any
datetime-range failure regardless of magnitude.
- _select_stale_candidates uses _safe_add_days(now, -effective_age); a None
result means the window is unrepresentably large, so the fact cannot yet be
stale and is skipped (not selected).
- Consolidation computes each source's deadline via _safe_add_days; a source
whose deadline overflows falls back to the global staleness_age_days deadline
(same treatment as a legacy no-evd source), so one malformed field cannot
abort the merge.
Normal values are unaffected - any legitimate expected_valid_days is far below
the overflow boundary (the config ceiling staleness_max_extension_days tops
out at 36500).
Tests:
- TestSafeAddDays: normal/negative shifts; 10**400/10**12/10**9 return None;
timedelta.max.days (the exact reproduced value) returns None, not raises.
- TestSelectStaleCandidates.test_huge_evd_does_not_abort_selection: a fact with
a huge evd is skipped, not selected, and selection does not raise.
- test_consolidation_with_huge_int_source_evd_does_not_raise now parametrized
over [1e400, 1e12, 1e9, timedelta.max.days] - the last is the value that
constructs a valid timedelta but overflows datetime arithmetic.
- Helper/read-path tests updated to assert huge ints are returned as-is (the
overflow guard is no longer in the helper).
8511fa6aa3