fix: name the row behind an unreadable date instead of internal_error - #247
Merged
Conversation
`stride season` walks every activity date and every daily_load day. One
unreadable value in either table took the whole command down with
internal_error — the arm whose message asks the user to open an issue.
Nothing here is unanticipated. The code constructs BadActivityDate and
BadDailyLoadDay on purpose, at a date it deliberately refuses to guess at;
they just never reached the boundary as themselves. So the envelope said
"unhandled failure: BadActivityDate(\"0000-0z-01\")" and told the user to
file a bug, when the actual remedy is a re-sync or a row repair.
Two codes, not one, because the remedies differ by TABLE rather than by
command. `activities.start_local` is mirrored from Strava and is repaired by
re-fetching; `daily_load.day` is derived by stride and is repaired by
rebuilding. A caller told only "a date is bad" cannot pick between them. The
daily_load message says `stride analyze`, not `stride analyze --all` — that
form does not exist and exits `usage`, which would be this same defect again:
an error whose remedy does not work. Measured against a poisoned snapshot
rather than read off the code: plain `analyze` DELETEs and rewrites the table,
and `season` succeeds afterwards.
The envelope now names the ROW, not just the date. A date is not something a
caller can act on; an id is. The season query carries MIN(a.id) out of each
group for that, and MIN rather than any because the group is (date, family) —
one bad date can hold several rows, and a bug report that quotes a different
id on every run is not a bug report.
The assertion that hid all of this was `Str.contains(out, "error")`, which
internal_error satisfies. It passed for two releases while `season` was
answering "please open an issue", because the e2e fixture has carried a
poisoned row since b_seed_analyze! and nothing calls `season` after that
point. The checks now assert the CODE, the quoted value, and the id — and the
remedy string, so a message naming a command form that does not exist fails
here.
Mutation-proved, each built clean and run:
- dropping the BadActivityDate arm at the boundary -> FAIL ...naming the
code, not internal_error
- example_id as a constant -> FAIL ...and naming the ROW
- the two codes swapped -> FAIL every error code the source emits is in the
contract, and vice versa
- MIN -> MAX -> FAIL ...and the LOWEST id when one bad date groups several
rows (this one survived until the shared-date row was added; with one row
per group MIN, MAX and "whichever" are the same value, so the choice was
asserted only by the comment claiming it)
653 -> 663 checks, exact.
Closes #243
…o not work
Review found the PR reproducing its own defect in three places. Every fix
below is measured, and the two behaviour changes are mutation-proved.
THE REMEDIES DID NOT WORK.
`stride analyze` was named as the fix for an unreadable daily_load day, and
it was a no-op in exactly the state that error is reachable from.
rebuild_daily_load! only reached its DELETE on the branch where at least one
activity date parsed; with none it returned Ok({}) and left the table as it
was. So the user ran the command the error named, got `converged: true` at
exit 0, and hit the identical error. Forever. That branch now clears the
table too — the other branch already DELETEs unconditionally, so this is the
same rule applied to the case where the walk has nothing to write.
The comment above it said "Verified end to end against a poisoned snapshot,
not read off the code". It had been — against a snapshot that happened to
hold parseable activities. The evidence was real and the sentence generalised
past it, which is the sentence that makes the next reader stop checking.
The activity message quoted a value no row contains. `raw` is
`substr(start_local, 1, 10)`, so "has start_local 'garbage-da'" sent the user
to a DELETE matching zero rows, and made every bug report quoting it
unreproducible. Carrying the whole column out was not available — SQLite's
bare-column rule only pins a value to the min/max row when there is ONE
min/max aggregate and this query has three — so the message now says
"beginning", and the id is the handle. The remedy order flipped too:
`sync --all` was measured to silently no-op on an imported row (synced_at
NULL, so the upsert never sees it and prune exempts it), so the remedy that
always works leads and the conditional one follows.
Both messages also drifted from the house shape — capitalised imperative,
terminal period, two sentences. Back to lowercase, one em dash, no period.
THE CLASS WAS NOT CLOSED.
`compare` ran the identical `SELECT day FROM daily_load ORDER BY day DESC
LIMIT 1` and collapsed the failure to epoch day 0. That does not fail, it
ANSWERS: a real 28-day block (138 TSS, 2 sessions, 58% easy) came back as
`has_data: false` with every figure 0 at exit 0, and the human line said "no
load recorded either 28d · fitness holding" — while `summary` refused on the
same database in the same run. An athlete told their fitness is holding by a
command that could not read the day it anchored on is the worst shape this
class has. It propagates now.
MIN(a.id) DID NOT MEAN WHAT ITS COMMENT SAID.
The grouping is (date, fam), so one bad date shared by a Run and a Ride is
two groups and MIN picks inside whichever the walk reaches first — not the
lowest id for the date. Measured: 900 (Run) and 901 (Ride) named 901. Adding
`fam` to the ORDER BY makes "first" our statement rather than SQLite's, so
one database names one row every time. The comment now says that instead of
claiming global minimality, and the new check pins it in the discriminating
direction: a LOWER id in a later-sorting family must not displace the answer.
FALSE COUNTS AND ENUMERATIONS, all corrected:
- "two tables that hold dates" — three; planned_sessions.target_date is the
one a user can poison without touching SQLite, via `week add`
- "three commands walk these two tables" — four can RAISE these tags, and
`walk` overclaimed: `load`, `stats` and `doctor` read them and absorb
- "the second time in this file a `contains` accepted the failure it was
written to catch" — at least the seventh, and nobody adding the eighth
would come here to update a number, so the ordinal is gone and the rule
stays
- "passed for two releases" — one; the check and `season` both arrived in
b25d3e9, which is in v0.7.0 and not v0.6.0
- the 401 arm was cited as precedent for SPLITTING codes; it kept one code
and widened the message, so it is the precedent for checking whether the
remedy is identical, not for splitting
- three comments quoted the pre-substr value; that slip is what put a
string no row contains into a user-facing "delete this" message
MUTATION-PROVED, each built clean and run:
- compare back to `.ok_or(0)` -> FAIL ...and `compare` refuses the same
anchor rather than answering with an empty month
- message back to claiming the column -> FAIL ...quoting the ten
characters it actually read
- analyze back to Ok({}) -> FAIL ...and `stride analyze`, the remedy it
names, really does clear the row
- GROUP BY date alone -> FAIL ...and a lower id in a later-sorting family
does not displace it
The analyze check needed its own database to bite: written first against the
shared fixture it passed with the fix reverted, because that fixture has
hundreds of parseable dates and so clears the row through the branch that was
never broken. It also needs all four hr_z*_max keys — with three, `analyze`
answers missing_config and the remedy never runs, which is the second way
that printed remedy can fail.
663 -> 673 checks, exact.
…ver data it dropped
Two reviews, independently, found the same three things. The first is the one
that matters: the previous commit's own sentence claimed the class was closed
and it was half open, inside the sentence declaring it.
THE GUARD WAS HALF A GUARD. `compare` and `summary` tested
`date_str_to_days` alone. That ACCEPTS "2026-3-05" — and the non-canonical
day is the DANGEROUS one, not the harmless one, because `ORDER BY day DESC`
is a string sort and "2026-3-05" beats every "2026-08-xx". Measured on a
two-row table:
summary -> as_of 2026-3-05, exit 0 (anchored on March against August)
compare -> has_data false, every figure 0, exit 0
season -> refused
Only season refused, because season was the one site also testing
is_canonical_date. Both anchors now go through one `canonical_day` helper
with both halves, so all four raising commands refuse the same day.
ANALYZE TRADED A LOUD FAILURE FOR A QUIET ONE. Clearing the table on the
no-parseable-day branch closed the loop the last commit was about, and opened
a smaller one underneath it. Two different facts arrive at that branch:
"nothing scored yet" and "rows exist and not one of their dates could be
read". Flattening both to Ok({}) meant the second answered `converged: true`
at exit 0, and `season` then said "no scored training days yet — run `stride
sync` then `stride analyze`" while `stats` reported the athlete's sessions
and kilometres in the same breath. That is the same defect one layer down: a
remedy that cannot work. It names the row now, carrying MIN(activity_id) out
of the rebuild query for the purpose.
The empty case still succeeds, and I checked the destructive direction: with
activity_metrics deleted, converge_metrics! rescores before the rebuild, so
the series comes back in the same run.
FALSE PROSE, again, and one of these is worse than the others.
- "`planned_sessions.target_date` ... the one a user can poison without
touching SQLite, since `week add` stores whatever string it is handed" —
false. plan_add! rejects a non-canonical date with `bad_date` before the
database is opened, and it is the only writer of that column, making it
the BEST-guarded date in the schema. The sentence cited Plan.roc's own
note as evidence; that note has been false since the day the guard landed
beside it. Prose sourced from prose, which is the mechanism #243 came
from.
- "`load`, `stats` and `doctor` ... still absorb" — only `load` does.
ReportHealth.roc holds both stats and doctor and contains zero
occurrences of daily_load and zero date parses; they compare start_local
as bytes in SQL, so an unreadable value joins or leaves a window by
string order. Different failure, named separately. And `load`'s 1969 row
is the rollup branch only (>14 days), stated conditionally now.
- "carrying the whole column out was not available" — not available FROM
THAT QUERY. A second read by id would return it; that is a round trip for
a string the user does not act on.
- "synced_at NULL, so the upsert never sees it" — wrong cause. The upsert
is keyed on ids from Strava's response and never consults synced_at;
synced_at is why PRUNE exempts the row. Stamping imports would change
nothing.
The `has_data == null` check is now paired with a positive marker. On its own
it is the vacuous-absence shape this file warns about twice: it also returns
null for a renamed field or any other failure of `compare`.
MUTATION-PROVED, each built clean and run:
- canonical_day back to parse-only -> FAIL ...and so does `summary`, on the
non-canonical day and not just the unparseable one
- analyze back to silently clearing -> FAIL scored rows with no readable
date make `analyze` refuse, not report converged
673 -> 678 checks, exact.
… reach
Two reviews, and the same headline as last round: the commit that said "close
the class" left more of it open. Round 3 fixed the sentence's subject; the
predicate had moved.
THE RAMP SERIES. `canonical_day` went on both ANCHOR reads and not on the
30-day ramp rows inside summary_payload!. The anchor guard cannot cover them,
because a non-canonical day only has to BE the anchor when it sorts highest.
Give the table a canonical day above it and the poisoned row slips into the
window untested — '2026-3-05' >= '2026-12-06' is true under byte order, and so
is '2026-3-05' < '2027-01-05'. Measured on those two rows:
before ramp_7d -49, ramp_28d_avg -12.25, form_delta -79,
form_delta_known TRUE, exit 0
honest 0, 0, 0, form_delta_known false
A fabricated ramp with a flag certifying it, which is verbatim what the fold's
own comment warns about, arriving through the guard just upgraded to catch it.
THE WRITER. `Analyze.roc`'s by_day fold guarded with the parse alone, and this
is the site where that matters most in the tree, because it WRITES. The parse
accepts "2026-3-05T", "2026-08-9T", "2026-8-05T"; the fold then writes the day
back through days_to_date_str, so it lands in daily_load looking perfectly
canonical. analyze said converged: true, summary and compare reported over the
invented day at exit 0, and season refused the same activity. One case walked
from March and produced 173 rows off a single malformed date.
No downstream guard can catch that — canonical_day inspects daily_load.day,
and by then the value has been laundered. The guard has to be upstream of the
write, so it is.
THE PARTIAL CASE, which is the likely one. The previous commit only refused
when NO date parsed. One bad row among many took the other branch and was
dropped silently: ten good activities plus one unreadable gave `converged:
true` and `computed: 11` beside ten days of load — two numbers in one payload
disagreeing, with nothing to say so. Every unusable row is now collected, the
walk still runs and still writes what it could read, and THEN the run refuses
naming the row. That order is deliberate: refusing first would leave the table
stale and every reader would keep answering confidently from it.
PROSE. The `synced_at` paragraph has now been wrong twice and is deleted
rather than rewritten a third time — the second version said stamping imports
"would change nothing", when prune_deleted! exempts exactly the NULL rows and
stamping them would move them INTO the victim set. The comment now carries the
consequence and leaves the mechanism where it lives. Also dropped a superlative
("best-guarded date in the schema"), and corrected a false parallel: Analyze's
`ORDER BY day, example_id` is belt-and-braces, not load-bearing like
ReportSeason's, because the group key there is (date, fam) and here it is day
alone.
MUTATION-PROVED:
- ramp rows back to parse-only -> summary publishes the -49 ramp at exit 0;
with the fix it refuses, on the reviewer's exact two-row fixture
- non-canonical activity date -> refused by the writer, and no invented day
reaches daily_load (both asserted)
- partial case -> refuses naming the row AND keeps the readable series
(both asserted; either alone is satisfiable the wrong way)
678 -> 682 checks, exact.
Two reviews again, and the class was still open — this is the third round in which a sentence claiming completeness turned out to be the false one. THE FIFTH SITE. `summary`'s hard-session statistics parsed ACTIVITY dates with `keep_oks` and `date_str_to_days`, which does both jobs badly: it drops an unparseable date silently AND accepts a non-canonical one, so the fold under-counts and mis-dates at once. Measured on a single row changed to the value `season` already refuses: hard_sessions.d14 1 -> 0 days_since_last 3 -> 172 days_since_known true -> true last_hard_session_date 2026-08-21 -> "2026-3-05T" exit 0 -> 0 (season refused the same row) A fabricated 172 carrying a flag that certifies it — the same shape the ramp fix removed one screen up in the same file — and 172 days against 3 is "badly overdue for intensity" against "recovering". That is the number a coach acts on. It propagates now, through a `canonical_activity_day` sibling of `canonical_day`, and the query carries MIN(a.id) so the refusal names a row rather than a date. `last_hard` was a second defect in the same block: a string MAX with no Roc-side parse, publishing the malformed value straight into a field the schema calls a date. ONE PREDICATE, ONE EXPRESSION. `usable_day` was defined and then not used by the fold ten lines below it — the same rule written twice in one function. They agreed only because both were typed correctly. A day excluded from `by_day` but not counted `unusable` is dropped silently, which is the #243 bug itself; one accepted but counted `unusable` makes the run refuse over data it did use. The fold calls the function now. Recording honestly that this one is NOT mutation-provable: replacing the call with the inline predicate is behaviour-equivalent today, so the suite stays green. That is exactly why it was worth fixing — the hazard is the second copy existing, not its current value. THE CHECKS WERE YEAR-DEPENDENT. Both new daily_load checks relied on '2026-3-05' outranking every fixture day under byte order, and the fixture's max day is roughly TODAY because rebuild_daily_load! extends the series through today. Review measured the 2027 fixture: `summary` and `compare` SUCCEED at exit 0 and both checks go red — failing by the command succeeding, which reads as the guard breaking. The day is constructed now (next year, unpadded month), and the fixture asserts both properties it depends on. The new hard-session check needed the opposite trick: it has to be non-canonical AND inside the 28-day window. An unpadded month is only non-canonical from January to September, so it is an unpadded DAY — every month has a 1st through a 9th — and the test asserts the constructed value differs from the padded spelling of the same day rather than assuming it. PROSE. The guard comment claimed to cover "every read of that column in this module"; `load_series!` does not go through it, deliberately, and app.roc says so in the same commit — two comments asserting opposite things about one column. Scoped to the property now, and the block went from 28 lines over a 6-line function to 16. Two paragraphs documenting this PR's own review history are deleted: that content is real and belongs here, in the commit message, where it is dated. A comment's reader needs the rule, not the changelog. MUTATION-PROVED: - hard-day fold back to keep_oks -> FAIL a non-canonical date on a HARD session refuses in summary rather than skewing its stats 683 -> 686 checks, exact.
Both reviews found the same sixth site independently, and one of its two
instances sat under a comment THIS commit's predecessor added saying "Guarded
below." It was not. The guard went on the 28-day fold; `last_hard` is
all-time, so every row the cutoff excludes was invisible to it.
Two string MAXes with no Roc-side parse at all, both measured:
last_hard_session_date "0000-0z-02" one malformed hard session older than
28 days with none inside the window —
a rest block, a taper, a comeback
sports_28d[].last_date "2026-3-05T" no hard_expr at all, so any poisoned
activity sorting above the cutoff
becomes its sport's "last seen"
Both at exit 0, on databases where `season` refuses the same row.
I tried guarding each read and it was wrong in an instructive way. MIN(a.id)
inside a GROUP BY names the lowest id IN THE GROUP, not the id of the row
whose date is the MAX — so guarding sports_28d.last_date that way named a
healthy activity and pointed the user at the wrong row. The e2e caught it;
the check that had just been written to demand a real id demanded the right
one.
So: one sweep over every activity date, before any of the reads. All-time,
matching `season`, which already refuses any activity with an unreadable
date — `summary` was the surface where the same row answered at exit 0.
Dropping the sweep now reds the check.
ONE PREDICATE, ONE PLACE. There were five independent spellings of
"canonical and parseable" — two in Report, two in ReportSeason, one in
Analyze — and every reopening of this class came from two implementations of
one rule with only one updated. What legitimately varies is the TAG, not the
rule, so `Metrics.usable_date_days` holds the rule and each site keeps its own
Err. It returns the DAY rather than a Bool, which removes a second parse whose
failure arm was unreachable — a silent-drop arm sitting in code waiting for
the predicate to be weakened.
THE EXIT CODE. Review conceded this one: `sync`'s `resumable: true` at exit 0
is meaningful because there is something to re-run into, and an unreadable
stored date has no such action. I am recording that I also gave a supporting
argument that was wrong — "exit 0 would make analyze the one command that
shrugs" is true only for a non-canonical date inside the window. For a date
that fails to parse outright, or one outside it, four commands shrugged and
two refused. That was measured, it was not what I claimed, and it is not in
any comment.
PROSE. "the last one in this module" is gone — a quantifier over sites, in the
same file as the sentence warning that every such quantifier has been false.
"Guarded below" is gone. The copies-list is gone with the copies. `hard_days`,
not `hard_sessions`, which was copied from a stale comment into a new one.
686 -> 688 checks, exact.
Rebased onto main (#242 and #245 landed; the check count resolves to 730). THE SEVENTH SITE, and it is the worst of them because it WRITES. `rate latest` resolved "latest" through `MAX(start_local)` — a byte comparison, so one malformed date outranks every real one. '2026-3-05T' beats '2026-08-24'. Measured: `stride rate latest 8` attached the rating to the malformed row, reported `{"rated":810}` back as the id it had rated, and exited 0 — on a database where `summary` and `season` both refuse. Two things make this worse in kind than the six read sites. It is durable, and it writes into `ratings` — the one table `prune_deleted!` refuses to touch, on its own stated grounds that a rating "can't be re-derived", so the row is human judgment that cannot be recovered. And it collides with the remedy every date error prints: deleting the malformed row by id also destroys the rating the athlete meant for a different session, which still has none. Not covered by summary's sweep — `rate!` is dispatched straight from app.roc and never reaches summary_payload!. It resolves by parsed day now, refusing rather than ranking around an unreadable date, because ranking around it is exactly how the wrong session got rated. Ties break on the higher id, which is what the string version meant and did whenever the dates parsed. ONE PARSE, FOR REAL THIS TIME. `usable_date_days` was written as `if is_canonical_date(s) date_str_to_days(s)` — and is_canonical_date is itself implemented over date_str_to_days, so it parsed twice and left the second Err arm unreachable. That is the shape the function's own comment condemns. It round-trips a single parse now: only one spelling of a day survives days -> string, which is what "canonical" means here. FIVE SPELLINGS TO ONE, ACTUALLY. Last round's claim was five to three: ReportSeason's two copies still called is_canonical_date inline. Both are one-line map_errs over the shared rule now. DEAD CODE WITH A FALSE COMMENT, introduced by me last round. `sports` was rebuilt field-for-field from `sports_raw` under a comment saying it refuses unreadable dates. It refused nothing — an identity map. Both found it; the safety was entirely upstream. Deleted. TWO CORRECTIONS TO MY OWN CLAIMS. The sweep does NOT run on `compare`: summary_payload! has exactly two callers, `summary!` and `plan_bundle!`. And the "Guarded below" paragraph, correctly deleted last round, left a live defect described in the present tense above a query that still had no guard of its own — it now says where the cover comes from. A leftover fixture row put the calendar back into the block written to be construction-safe: activity 803 outlived its own check and decided by byte order which row the sweep named. Deleted at the point it stops being needed. MUTATION-PROVED: - rate latest back to a string max -> FAIL `rate latest` refuses an unreadable date rather than rating the wrong session - usable_date_days drops the round-trip -> FAIL a non-canonical activity date is refused by the WRITER, not laundered into a real-looking day 727 -> 730 checks, exact. Every sync arm green. issue-claims clean.
eschizoid
force-pushed
the
fix/243-bad-activity-date
branch
from
August 24, 2026 12:49
902ce6a to
ba0ada5
Compare
Two regressions, both mine, both in the commit I pushed without review, and
both found by the review I should have asked for first.
THE TIE-BREAK CLAIM WAS FALSE. I wrote that ranking on the parsed day was
"what the string version meant and did do whenever the dates parsed". It was
not. `MAX(start_local)` compares the WHOLE timestamp, so two sessions on one
day never tied — the later won outright, and MAX(id) only ever separated
identical stamps. Truncating to the day manufactures a tie that did not exist
and then resolves it by id, which has no relationship to time of day:
id=800 2026-08-24T18:00:00Z evening
id=900 2026-08-24T08:00:00Z morning
old query picked 800. the day-ranked fold picked 900.
So after a two-a-day, `rate latest` rated the MORNING session whenever it held
the higher id. Ids track upload order, which usually tracks time and does not
have to — a backfilled ride, a manual entry, an import all break it. And
`rate latest` is exactly what gets run after training, on exactly the day two
sessions exist. Same failure as the one this PR fixed, one size smaller.
The shape is better now, too: Roc GUARDS, SQL RANKS. The guard sweeps every
activity date grouped by date; the original query then does the ranking it was
always correct about. It was only ever wrong because an unreadable date could
outrank a real one, and now none can reach it.
THE YEAR BOUND. `usable_date_days` was rewritten to round-trip a single parse,
and dropped `c.y >= 1000 and c.y <= 9999` on the way. That is not cosmetic:
`date_str_to_days` parses the year with arg_i64, which accepts any integer,
and `days_to_date_str` emits it UNPADDED — so "999-01-01" round-trips cleanly,
and sorts ABOVE every real date under `ORDER BY day DESC`. Measured: `summary`
anchored on year 999 and reported it as `as_of`, at exit 0. That is the
fabricated-anchor bug this branch spent four rounds closing, reopened through
the guard that closes it.
It was provable inside the same binary without a rebuild: `week add
999-01-01` still answered `bad_date`, because plan_add! calls
is_canonical_date, which kept the bound. Two live definitions of "canonical",
opposite answers on one string — in the commit whose message said "FIVE
SPELLINGS TO ONE, ACTUALLY". The commit framed the rewrite as an optimisation
and did not mention the narrowing loss, and no check caught it.
Bound restored, and `is_canonical_date` is now DEFINED OVER `is_usable_date`
rather than beside it, so there is one body rather than two agreeing by
coincidence.
MUTATION-PROVED, both with new checks:
- rank on the day again -> FAIL ...and on a two-a-day rates the LATER
session, not the higher id
- drop the year bound again -> FAIL a year-999 day is refused, not accepted
as canonical because it round-trips
The second check has a sibling asserting `week add` refuses the same year, so
the two definitions cannot silently diverge again.
Also: "the one table prune_deleted! refuses to touch" is two —
planned_sessions gets the same protection on the same grounds. Sixth
count-over-sites on this PR that measurement has falsified, and it was in the
sentence carrying the argument.
730 -> 733 checks, exact.
The last commit's two halves pulled against each other, and review found the
gap between them. The tie-break fix was right that ranking must use the whole
timestamp. The guard only certified the first ten characters. A guard whose
domain is narrower than its consumer's leaks through the difference:
id=800 2026-08-24T18:00:00Z the real evening session
id=810 2026-08-24T37:00:00Z valid date part, impossible hour
guard both substr(1,10) = '2026-08-24', canonical -> PASSES
ranker MAX(start_local) -> T37, because '3' > '1'
result {"rated":810}
So a malformed TIME on a valid DATE took the rating. That is the same
wrong-session write this PR opened on, arriving one commit after fixing it,
through the seam between "Roc guards" and "SQL ranks".
The row shape is not hypothetical. Metrics.export_date_to_iso is documented to
have produced exactly T37 from "25:00:00 PM" before its components were
range-checked, and a database carrying those rows is the one this command
exists to repair.
The guard now groups by the WHOLE start_local and checks every field the
ranker's byte comparison depends on: date canonical, separator, both colons,
and hour/minute/second as two digits in range. Two digits matters on its own —
a one-digit hour sorts wrong against a two-digit one, which is the property
being protected rather than a formatting preference.
Split in SQL rather than Roc, because Str has no slicing here and substr keeps
the guard reading literally the same expression MAX sees.
AND THE COALESCE IS BACK. I dropped it in the rewrite, so a NULL start_local
failed the DECODE with UnexpectedType(Null) — `internal_error`, "please open an
issue" — on a write path. One commit earlier the same database answered
`unreadable_activity_date` and named the row. Narrowing a good error into the
exact shape #243 was opened to remove is the worst kind of regression this
branch can produce, and it was a one-token edit.
MUTATION-PROVED:
- guard the date only -> FAIL an impossible HOUR on a valid date is refused,
not ranked above a real session
- drop the COALESCE -> FAIL a NULL start_local is named, not answered with
internal_error
The first check clears `ratings` immediately before probing, so "nothing was
rated" means the refusal wrote nothing rather than the table happening to be
empty — which is the distinction the whole block is about.
Also re-wrapped the comment line the previous correction left at 133 columns.
That is twice now that fixing a prose finding produced a new one by splicing
without re-wrapping.
733 -> 736 checks, exact.
Review's diagnosis, not its symptom: each fix on this branch was scoped to the
failure that was demonstrated rather than to the invariant behind it. The
invariant is one sentence — the guard's domain must equal its consumer's — and
it had been re-derived by hand four times (day vs day, day vs timestamp, ten
chars vs whole string, nineteen vs whole string), coming up short every time.
So it is structural now. The guard validates substr(start_local, 1, 19) and
the ranker compares substr(start_local, 1, 19). Not two lists that have to
agree — the same expression. Positions 1..19 are the whole date and time, so
nothing past them can reorder rows that differ, and now nothing past them can
reorder rows that do not either.
The eighth component was real: a lowercase 'z' in position 20 outranked an
uppercase 'Z' on an identical timestamp, silently overriding the documented
tie-break that says MAX(id) decides those. Bounded — it could not make an
earlier session outrank a later one — but it was unchecked, and ranking on the
slice deletes the whole class rather than adding a condition.
TWO HALVES, SPLIT BY WHAT EACH LANGUAGE DOES CHEAPLY. Grouping by the whole
timestamp made the sweep scale with ACTIVITIES rather than DAYS: review
measured ~870ms at 50k against ~86ms for the day grouping, and found their own
round-8 number had been taken on a fixture where distinct timestamps happened
to equal distinct days. The date half needs Roc — the round trip and the year
bound are not SQL-expressible — so it groups by day. The time half is pure
comparison and stays in SQL as one offending-row query, costing nothing per
row.
A DEAD GUARD DELETED. I wrote `start_local IS NULL OR NOT(...)` in the time
predicate and then mutation-tested it: removing the NULL arm changed nothing,
because the date sweep gets there first and its COALESCE turns NULL into "".
A guard that cannot fail is what this branch has spent nine rounds deleting,
so it is gone. The ordering it depended on is now stated and proven — neutering
the date sweep reds `a NULL start_local is named`.
THE MESSAGE NAMED THE HALF THAT WAS FINE. "beginning '2026-08-24'" for a row
whose date is perfectly readable and whose fault is T37. That is round 1's
defect — quoting a value the column does not hold — wearing new clothes:
quoting the half that is correct. It names the failing component now, and
"beginning" is gone with it, because the time slice does not begin anything.
MUTATION-PROVED:
- rank on the whole string -> FAIL a byte past position 19 cannot override
the tie-break
- neuter the date sweep -> FAIL a NULL start_local is named, not answered
with internal_error
Verified against all eight forms review probed: lowercase t, hour 24, second
60, one-digit hour, bare date, NULL and T37 all refused; fractional seconds
and a real 18:00 session still rank correctly.
736 -> 738 checks, exact.
eschizoid
added a commit
that referenced
this pull request
Aug 24, 2026
Taking the `not_set` finding at its strict reading broke the case the skip machinery exists for. `stride init` writes ZERO config rows, so on a fresh install every key answers `not_set` — and removing it from the allowlist made `just schema-check` exit 1 on a correct, uncorrupted new database, with a message claiming the invocation had been rejected. Both halves false. It is back, under protest, with #254 owning its removal. The asymmetry decides it: the hole it leaves needs a FUTURE rename of the `timezone` key to bite, while the breakage bit immediately, on precisely the state the skips are for. A checker that cries wolf on a clean install is the mirror of the hole this PR opened on — a check nobody trusts is as useless as one that always passes. AND THE DATABASE ARM IS BACK. Folding #183's `no_database` case into the catch-all meant a corrupt database printed "the derived invocation was rejected, not the database" — sending the reader hunting for a bug in this recipe while their database is the problem. The comment describing that arm was still sitting above the code that had deleted it, which is comment drift inside the same edit. Three outcomes now, and they are genuinely different things: skipped nothing to say on this database (fresh install, no activities) FAILED the DATABASE holds a value the engine cannot read FAILED the INVOCATION was rejected — this recipe's own bug The #247 codes join the second arm rather than the skip list, and the reason is coverage rather than principle: `season`, `summary`, `plan` and `compare` can all raise them, so allowing them to skip would silently drop four of eighteen forms — 22% — on exactly the databases where payload validation matters most. They are also argument-independent, so failing on them can never be a false accusation caused by this recipe's own derived filler. VERIFIED on all three shapes: fresh install 0 FAILED lines, exit 0 corrupt database FAILED (corrupt_database) — the database holds a value the engine cannot read poisoned date FAILED (unreadable_daily_load_day), exit 1 real database 18 forms, all conform, exit 0
eschizoid
added a commit
that referenced
this pull request
Aug 24, 2026
Taking the `not_set` finding at its strict reading broke the case the skip machinery exists for. `stride init` writes ZERO config rows, so on a fresh install every key answers `not_set` — and removing it from the allowlist made `just schema-check` exit 1 on a correct, uncorrupted new database, with a message claiming the invocation had been rejected. Both halves false. It is back, under protest, with #254 owning its removal. The asymmetry decides it: the hole it leaves needs a FUTURE rename of the `timezone` key to bite, while the breakage bit immediately, on precisely the state the skips are for. A checker that cries wolf on a clean install is the mirror of the hole this PR opened on — a check nobody trusts is as useless as one that always passes. AND THE DATABASE ARM IS BACK. Folding #183's `no_database` case into the catch-all meant a corrupt database printed "the derived invocation was rejected, not the database" — sending the reader hunting for a bug in this recipe while their database is the problem. The comment describing that arm was still sitting above the code that had deleted it, which is comment drift inside the same edit. Three outcomes now, and they are genuinely different things: skipped nothing to say on this database (fresh install, no activities) FAILED the DATABASE holds a value the engine cannot read FAILED the INVOCATION was rejected — this recipe's own bug The #247 codes join the second arm rather than the skip list, and the reason is coverage rather than principle: `season`, `summary`, `plan` and `compare` can all raise them, so allowing them to skip would silently drop four of eighteen forms — 22% — on exactly the databases where payload validation matters most. They are also argument-independent, so failing on them can never be a false accusation caused by this recipe's own derived filler. VERIFIED on all three shapes: fresh install 0 FAILED lines, exit 0 corrupt database FAILED (corrupt_database) — the database holds a value the engine cannot read poisoned date FAILED (unreadable_daily_load_day), exit 1 real database 18 forms, all conform, exit 0
eschizoid
added a commit
that referenced
this pull request
Sep 6, 2026
A review agent was asked to attack exactly this and found it. Converting "used to crash" into "crashes" asserts something about the CURRENT code, and where the defect was genuinely fixed upstream the present tense is false. Five sites, one systematic error: - ReportSessions, Plan, and e2e each said an interpolated empty string "crashes" str_concat. roc#10595 closed 2026-08-04, before this pin. The ReportSessions one contradicted itself inside twelve words — "crashes ... fixed upstream and carried by the current pin" — and the Plan one disagreed with its own sibling comment twelve lines above. - both Strava copy notes said wrapping bindings "crashes real sync". The app is on basic-cli 0.22.0; the crash was bug C on 0.21, and nobody has re-tried the wrap since. The notes now carry the MECHANISM instead of a bare prohibition — interpolation makes every binding a heap Str, which is what the 0.21 host double-freed — so the copy is now pointless rather than fatal, and the note says why it still stays. Three claims that were simply backwards or wrong: - the checks_ran_at_least! guard: I deleted the drifting numbers and took the DIRECTION with them. The check asserts `ran >= floor`, so an actual BELOW the floor fails — the one case that cannot hide anything. Slack is a floor far below the actual, which is now what it says. - Command.roc claimed the union check is satisfied "while `load` does not" declare the code. It does, four lines below, and e2e asserts it by name. The structural point survives as a counterfactual. - app.roc said the database read "fits inside the message closure" while the sentence before it says to read it BEFORE the closure — which is what the code does. Two counts and one attribution: - "eight other queries order on the whole column with no guard" is the pre-#247 tree. Nine ranking sites now emit the clause; the sentence is scoped to when the issues opened. - the re-fetch promise belongs to the `is_bookkeeping` arm, not the catch-all, and the comment 27 lines below already said the catch-all deliberately avoids that promise. Two comments in one match contradicted. - ADR 0003 records `model_<sport>` as shipped differently; it carries `threshold_pace_<sport>` as live, which shipped as a stored derivation. My rewrite had applied one key's fate to both, and its head ("waiting to enter that state") denied its own tail ("none is planned"). Found while checking that last one, and NOT in the review: Config.roc still said the cost of a missing entry "went up when `config set <key> ""` became a DELETE". #276 retired that gesture — config_store! refuses an empty value and config_delete! is reached only from `config unset`. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
eschizoid
added a commit
that referenced
this pull request
Sep 6, 2026
A review agent was asked to attack exactly this and found it. Converting "used to crash" into "crashes" asserts something about the CURRENT code, and where the defect was genuinely fixed upstream the present tense is false. Five sites, one systematic error: - ReportSessions, Plan, and e2e each said an interpolated empty string "crashes" str_concat. roc#10595 closed 2026-08-04, before this pin. The ReportSessions one contradicted itself inside twelve words — "crashes ... fixed upstream and carried by the current pin" — and the Plan one disagreed with its own sibling comment twelve lines above. - both Strava copy notes said wrapping bindings "crashes real sync". The app is on basic-cli 0.22.0; the crash was bug C on 0.21, and nobody has re-tried the wrap since. The notes now carry the MECHANISM instead of a bare prohibition — interpolation makes every binding a heap Str, which is what the 0.21 host double-freed — so the copy is now pointless rather than fatal, and the note says why it still stays. Three claims that were simply backwards or wrong: - the checks_ran_at_least! guard: I deleted the drifting numbers and took the DIRECTION with them. The check asserts `ran >= floor`, so an actual BELOW the floor fails — the one case that cannot hide anything. Slack is a floor far below the actual, which is now what it says. - Command.roc claimed the union check is satisfied "while `load` does not" declare the code. It does, four lines below, and e2e asserts it by name. The structural point survives as a counterfactual. - app.roc said the database read "fits inside the message closure" while the sentence before it says to read it BEFORE the closure — which is what the code does. Two counts and one attribution: - "eight other queries order on the whole column with no guard" is the pre-#247 tree. Nine ranking sites now emit the clause; the sentence is scoped to when the issues opened. - the re-fetch promise belongs to the `is_bookkeeping` arm, not the catch-all, and the comment 27 lines below already said the catch-all deliberately avoids that promise. Two comments in one match contradicted. - ADR 0003 records `model_<sport>` as shipped differently; it carries `threshold_pace_<sport>` as live, which shipped as a stored derivation. My rewrite had applied one key's fate to both, and its head ("waiting to enter that state") denied its own tail ("none is planned"). Found while checking that last one, and NOT in the review: Config.roc still said the cost of a missing entry "went up when `config set <key> ""` became a DELETE". #276 retired that gesture — config_store! refuses an empty value and config_delete! is reached only from `config unset`. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #243.
stride seasonwalks every activity date and everydaily_loadday, so it is the command that meets a bad one. A single unreadable value took the whole command down withinternal_error— the arm whose message asks the user to open an issue. Nothing here is unanticipated: the code constructsBadActivityDateandBadDailyLoadDayon purpose, at a date it deliberately refuses to guess at. They just never reached the boundary as themselves.Two codes, not one, because the remedy depends on the TABLE rather than on which command met the row:
activities.start_localis mirrored from Strava and is repaired by re-fetching,daily_load.dayis derived by stride and is repaired by rebuilding. The envelope names the row, not just the date — a date is not something a caller can act on.What review turned this into
The PR opened on one command and one code. Review found the same class in six more places, and each round found something the previous round's fix had missed. The honest summary is that the class was much larger than the issue, and the last four rounds were spent on defects in my own fixes.
stride analyze, named as the remedy, was a no-op in the state that error is reachable from — the user ran it, gotconverged: trueat exit 0, and hit the identical error forevercompareandsummaryguarded with the parse alone, which accepts2026-3-05— and byte-order sorting makes the non-canonical day the anchoranalyzeturned2026-3-05Tinto a canonical-looking2026-03-05that every downstream guard then trustedsummary's hard-session stats publisheddays_since_last: 172against a true 3, underdays_since_known: trueMAX(substr(...))reads with no parse at all, publishing malformed values into fields the schema calls datesrate latest— a write — attached an unrecoverable rating to the wrong activityThe invariant, made structural
Round 10 is the one that mattered, and the reviewer's diagnosis is why:
The invariant is one sentence — the guard's domain must equal its consumer's domain — and it had been re-derived by hand four times, coming up short every time. It is now the same expression on both sides: the guard validates
substr(start_local, 1, 19)and the ranker comparessubstr(start_local, 1, 19).The date rule went the same way. Five independent spellings of "canonical and parseable" collapsed into
Metrics.usable_date_days, withis_canonical_datedefined over it rather than beside it — because two bodies of one rule is how the year bound was lost from one and not the other, in a binary whereweek add 999-01-01and every stored-date guard then disagreed about the same string.Verified
Rounds 1–8's regression set re-run against the final commit: every intermediate defect reproduces as a clean refusal naming an actionable row, and
analyzekeeps the readable series rather than discarding it over one bad row.Performance: grouping the sweep by whole timestamp scaled with activities rather than days. Split by what each language does cheaply — date half in Roc grouped by day, time half a single SQL query — it is ~205ms at 50k activities against ~870–1060ms, and a few milliseconds at real scale.
Every fix is mutation-proved. Two were found by mutating my own fix before shipping it: the slice equality was load-bearing and unchecked, and a
start_local IS NULL OR NOT(...)arm I wrote was dead — the date sweep gets there first.663 → 738 checks, exact.
Filed rather than fixed
loadprinting a fabricated 1969 weekconfig getcannot tell an unknown key from an unset onestart_localwith no guarantee it is rankable, withPlan.candidate_activities!called out as the one on a write path (unmeasured, so: same seam, not a demonstrated wrong write)