docs: correct false claims about what the code can and does do - #374
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Two updated doc comments remain internally inconsistent/ambiguous (ReportHealth “are here” wording and Config “waiting keys” wording), which undermines the PR’s goal of making comments reliably accurate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR corrects verified-false internal documentation/comments so they accurately describe current behavior and architecture, reducing the risk of readers chasing the wrong invariants or failure modes.
Changes:
- Clarifies Strava drain progress-bar narration behavior (message tail vs bar width).
- Updates module header docs in ReportHealth and explanatory comments in Analyze/Metrics/Config to match the current config/model behavior and ADR 0003’s “shipped differently” reality.
- Removes/reframes a misleading memory-safety rationale in Analyze in favor of the actual
Err(NoRowsReturned)behavior described where config is loaded.
File summaries
| File | Description |
|---|---|
| src/Strava.roc | Refines stderr/progress-bar framing comment to describe the general failure mode. |
| src/ReportHealth.roc | Updates header to include pace-curve and corrects helper-placement explanation. |
| src/Metrics.roc | Corrects commentary about per-sport FTP config vs derived-FTP refusal behavior. |
| src/Config.roc | Aligns comment about model_<sport> with ADR 0003’s “no such key” reality. |
| src/Analyze.roc | Replaces incorrect “heap corruption” rationale with the actual missing-row reason already documented at config-load time. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
+40
| # not read it" — silent and destructive, not annoying and visible. Two | ||
| # documented keys are waiting to enter that state: `threshold_pace_<sport>` and | ||
| # `model_<sport>`. Whoever wires one up MUST add it here in the same commit. | ||
| # `model_<sport>`, which ADR 0003 records as SHIPPED DIFFERENTLY — there is no such | ||
| # key and none is planned. If that ever changes, it has to be added here in the same | ||
| # commit as the key itself. |
Comment on lines
+14
to
+18
| # Two are NOT shared and are here only by adjacency: Report.sport_exact_sql and | ||
| # Report.sports_with_speed! serve pace-curve alone. They sit beside sport_filter_sql | ||
| # because they are the same concern — resolving a sport word to a SQL predicate — and | ||
| # splitting one across two files would be worse than the rule they break. Stated rather | ||
| # than left for a reader to notice the rule does not hold. |
There was a problem hiding this comment.
🟡 Changes recommended
Two newly edited comments in tests/e2e.roc introduce or retain incorrect phrasing (“most schemas” enum claim, and an inverted floor/slack example) that should be corrected to keep the PR’s “docs correctness” goal intact.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
| check!("the declared error-code set is non-empty (got ${ndecl})", ndecl != "" and ndecl != "0")? | ||
| # The selector is PINNED to the error-code enum's path rather than sweeping | ||
| # every enum: enums are the house style (11 of 26 schemas carry one), and | ||
| # every enum: enums are the house style (most schemas carry one), and |
Comment on lines
7380
to
7383
| # A floor, NOT an exact count — `>=`, so setting one to today's count does not | ||
| # make it exact: adding checks never fails it. It must be TIGHT (a floor of 400 | ||
| # against 564 actual lets 164 checks vanish silently); a tight | ||
| # against a smaller actual lets checks vanish silently); a tight | ||
| # floor only needs touching when checks are REMOVED, exactly the event that |
There was a problem hiding this comment.
🔵 Needs a closer look
A few updated comments in tests/e2e.roc still contain (or introduce) inaccurate/brittle claims and minor grammar issues that should be corrected to match the PR’s stated goal of eliminating verified-false assertions.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
tests/e2e.roc:1102
- Minor grammar: "the 14 the default arm expects" is missing a noun, which makes the sentence harder to read.
This issue also appears on line 6931 of the same file.
tests/e2e.roc:7383
- This comment now says "a floor of 400 against a smaller actual", but in this example the problem is that the floor is smaller than the actual check count (so missing checks can slip by). Reword to avoid implying the actual count is smaller, and consider removing the hardcoded example number to prevent future drift.
# A floor, NOT an exact count — `>=`, so setting one to today's count does not
# make it exact: adding checks never fails it. It must be TIGHT (a floor of 400
# against a smaller actual lets checks vanish silently); a tight
# floor only needs touching when checks are REMOVED, exactly the event that
tests/e2e.roc:6934
- The updated comment still hardcodes a partial count ("199") and now reads awkwardly across the line break ("and 199 / of the call sites"). Since this PR is removing rotting/false counts, it would be better to drop the number entirely and keep the sentence grammatical.
# The failure log: a failing write used to be invisible three times over
# (sqlite3 reports on stderr, sh! discards stderr AND the exit code, and 199
# of the call sites discard the return) — it surfaced later as an
# unrelated-looking assertion about state.
tests/e2e.roc:3106
- "most schemas carry one" appears to be inaccurate (there are 37 schemas under schemas/v3, and only a minority contain an enum). Since this comment is justifying why the selector is pinned, it can stay correct and drift-resistant by removing the quantifier.
# The selector is PINNED to the error-code enum's path rather than sweeping
# every enum: enums are the house style (most schemas carry one), and
# envelope.json is a likely home for a second whose members would be wrongly
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
One updated comment in tests/e2e.roc still makes a verifiably false claim (“most schemas carry one” enum) and should be corrected to avoid reintroducing drift.
Review details
Suppressed comments (1)
tests/e2e.roc:3105
- This comment says “most schemas carry” an enum, but
schemas/v3/currently has 37 schema files and only 13 contain an"enum"keyword, so this is factually incorrect (and likely to drift again). Consider removing the quantitative claim and keeping just the rationale for pinning the selector.
# every enum: enums are the house style (most schemas carry one), and
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
src/Config.roc now asserts model_<sport> “does not exist / none planned,” which conflicts with existing in-repo documentation (Metrics.model_key) and should be clarified to avoid contradictory guidance.
Review details
Suppressed comments (1)
src/Config.roc:40
- The updated
model_<sport>note says “there is no such key and none is planned”, but the repo still documentsmodel_<sport>as an intended config key inMetrics.model_key(used to build the key name). This creates conflicting in-repo documentation and makes it unclear whether the key is merely unimplemented vs explicitly out of scope per ADR 0003.
Consider rephrasing to clarify that model_<sport> is named in code/comments but is not currently read by any command, and only becomes subject to Config.known_key if/when it’s actually wired up.
# documented keys are waiting to enter that state: `threshold_pace_<sport>` and
# `model_<sport>`, which ADR 0003 records as SHIPPED DIFFERENTLY — there is no such
# key and none is planned. If that ever changes, it has to be added here in the same
# commit as the key itself.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A newly edited e2e-harness comment still makes an absolute “suite never uses localhost / every base string is 127.0.0.1” claim that is contradicted by the probe directly below it, which undermines the PR’s goal of eliminating verified-false commentary.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/e2e.roc:1589
- This comment contradicts the probe immediately below: the suite does exercise the
localhostspelling here, and the repository now containsSTRIDE_API_BASE=http://localhost:…. Reword to avoid the false “suite never uses” / “every base string is 127.0.0.1” absolute claim while keeping the rationale about why thelocalhostdisjunct matters.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
One updated comment in src/Drain.roc is internally inconsistent (“wider by two inhabitants” but “a fifth arm”), and should be reworded to avoid reintroducing a misleading claim.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Drain.roc:37
- The comment says SyncStop is "wider than StopReason by two inhabitants", but then refers to "a fifth StopReason arm". Since there are two list-only inhabitants (list_rate_limited and list_daily_cap_reached), it’s clearer to avoid a specific ordinal here and just say “adding list-only arms”.
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A few updated comments are internally contradictory or conflict with existing in-repo documentation (e.g., “crashes” vs. “fixed upstream”), so they should be corrected to avoid reintroducing false claims.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
477
to
478
| # empty string crashes this backend in str_concat (roc#10595, | ||
| # fixed upstream and carried by the current pin). Non-empty by construction is |
Comment on lines
+541
to
+544
| # Q1: a per-sport override falls back to the GLOBAL bound — when one exists. Read that | ||
| # before the closure, beside `existed`, and let the message capture it: the payload and | ||
| # `config_unset.json` stay a two-field `{key, removed}`. I had deferred this claiming it | ||
| # "needs a database read inside the message closure, a different shape"; the read fits | ||
| # here, and the claim was wrong. | ||
| # `config_unset.json` stay a two-field `{key, removed}`. The database read fits | ||
| # inside the message closure. |
| _ = sql!(ctx.db, "DELETE FROM activity_segments WHERE activity_id IN (330,331,332,333,334); DELETE FROM activity_metrics WHERE activity_id IN (330,331,332,333,334); DELETE FROM activities WHERE id IN (330,331,332,333,334);") | ||
| # --help rather than a bare call: interpolating a compile-time empty string | ||
| # into the command slot is the #32-class crash, and --help returns the | ||
| # into the command slot crashes str_concat (roc#10595), and --help returns the |
The module header listed "doctor, stats, zones, power-curve, tte" and described "the power-duration curve". `pace_curve!` has lived in this file since #360 and appeared in neither. I added the command and did not update the file's own table of contents, which is the first thing a reader meets. `Strava` described "the tail of a 138-character message" welded beside a 33-character bar. The 138 is illustrative, and it is also wrong — the message it points at measures 142-144. The failure has nothing to do with that number: any message longer than the bar does it. It now says that. First changes verified under the toolchain the repo actually pins (nightly-2026-09-04-c125b82); everything before this on #365 was measured against nightly-2026-08-27 because the pin moved mid-branch. Refs #365 Build rc=0, four gates rc=0, suite 1111 == 1111.
`Metrics` said swimming, soccer and paddleboard "just work once configured". They cannot be configured that way: `Config.is_derived` refuses every `ftp_<sport>` key, including sports that do not exist yet. The sports do score — through the HR fallback — which is the point the sentence was reaching for and now makes. `Analyze` justified skipping a per-key DB read with "that path corrupts the heap here". It does not. The same file gives the real reason 100 lines above: a query for an absent override returns Err(NoRowsReturned) and would fail the command on the first missing key. A correctness problem described as a memory-safety one sends the next reader hunting the wrong class of bug. It now points at the existing explanation rather than restating it — restating it was my first fix, and duplicating a rationale is the defect P5 of this ticket exists for. `Config` instructed a maintainer that whoever wires up `model_<sport>` must register it there. ADR 0003 records that as shipped differently: there is no such key and none is planned. The instruction stays, conditional on that ever changing, instead of implying work is pending. `ReportHealth`'s header stated a placement rule — helpers stay in Report.roc only when shared with another family — and named three. Two more live there and are NOT shared: `sport_exact_sql` and `sports_with_speed!` serve pace-curve alone. I put them there in #360 and left the rule claiming otherwise. The header now says five, why the two break the rule, and why that is still the right call. Refs #365 Build rc=0, four gates rc=0, suite 1111 == 1111.
Batched rather than one at a time. Extracted every comment carrying a count-shaped number, filtered out domain values — fixture ids, HTTP codes, per_page, sample counts, TSS figures are properties, not counts — and checked what remained against the tree. DELETED, because nothing asserts them and they were all already wrong: "15 of 19 forms" (38 pinned verbs), "seven mock instances", "27 emit sites" (90), "11 of 26 schemas carry an enum", "277 call sites" (598), "against 564 actual lets 164 checks vanish" (1111), "check 2 of 851", "219 block expects" (225), "105 references across 12 files". Each was making a MAGNITUDE argument — most forms, every emit site, a smaller actual — and the argument survives the figure. Correcting them would only reset the clock on the same drift, and I already introduced one wrong replacement number earlier in this ticket by doing exactly that. CORRECTED, because a witness in the file disagreed with them: "120 samples @200W" -> 1300. The function's own comment three lines below reads "1300 samples (>= the 1200-sample best_20min_w window)". "the 12 the default arm expects" x2 -> 14. The literals two lines beneath each comment are 14. Where a comment and the code beside it disagree, the code is the witness and the number is worth fixing rather than dropping. Refs #365 Build rc=0, four gates rc=0, suite 1111 == 1111.
A comment telling a future reader to do something already done is worse than a wrong number: it invites the work twice. - threshold_pace_key carried "WHEN THAT SLICE LANDS: add this family to Config.known_key in the SAME commit". The slice landed — Analyze.period_threshold_sql derives the per-sport threshold and stores threshold_pace_used — while threshold_pace_key kept zero production callers and Config.roc records that no such key exists or is planned. The instruction is gone and the outcome recorded. - the paragraph under it warned that `config set <unrecognised> ""` DELETES the row. #276 retired that gesture; config_delete! is now reached only from config_unset!. Deleted rather than corrected, since the hazard it warned about no longer exists. Gates described more narrowly than they are: - half-efficiency has no per-half coverage rule. The single gate is on the whole signal span (>= 570 s and >= half the session), so "under 5 min per half is Unknown" was false at the boundary: 570 s is 4.75 min per half and returns Known. Also corrected against their witnesses: the 95% factor lives in one Roc constant AND four SQL literals; no caller ranks on a bare `start_local`, so that sentence is insurance rather than a live site; events_screen draws no table and binds no legend, emitting indented rows with the inputs in its lead line and base_note. Dropped a self-refuting clause: the reps census claimed "the other one" agrees in number, but the mixed fixture has three matches and asserts the PLURAL ("the other 2"). The singular branch is exercised by no expect, so the clause asserted a symmetry nothing tests. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Two comments claimed a guarantee stronger than the code provides. - Analyze's input constants: "the SELECT that stores the value and the predicate that compares it both interpolate these, so the write and the check cannot drift apart" holds for nine of the eleven. `e_sport` and `e_start` appear only in inputs_changed_sql; the write spells them separately as the `:usport`/`:ustart` bindings — which is exactly the drift the sentence said was impossible. The exception is now named. - Db.sport_ftp!: "keeps the stored ftp_used and the invalidation CASE exactly equal" named the wrong construct and the wrong direction. sport_ftp! is a display path (its callers are `zones` and `activity`); it writes no ftp_used and feeds no invalidation. The FTP half of the predicate is a CAST comparison, not a CASE — the only CASE in pending_where is the zones one. Restated as what the rounding actually buys: the screen cannot show a figure the check would disagree with. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
"NP requires at least 30 samples" headed the rank_ts_sql block, whose next line starts a completely different subject. The expect it describes sits 38 lines further down, with the rank_ts_sql and two_digit_in comments and expects in between. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Five blocks in Command.roc narrated a past defect where the present-tense property is shorter and checkable. The rule survives; the story goes. - the `load` error declaration carried five lines about a gap nothing caught. What a reader needs is the mechanism: the e2e union check asserts the UNION of declared codes across forms, so a form's omission is masked by neighbours declaring the same code. That is a property of the check, true for any future form, and it is now what the comment says. - `week`'s declaration explained that it "had NO declared codes at all, and now has one". It refuses an unplanned activity whose date it cannot read; the count of codes it used to have changes nothing. - `--json`'s rationale opened on what the output "used to" be. Stated forward: it answers with typed entries because an agent needs the argument shape, the write/network flags, and the schema. - `reps asc` and `progress desc asc` both described what the parser once let through. Both are now the requirement — a sort word in the date position must be refused at parse time — with the observed bad output kept only as the reason refusing matters. Deleted outright: "This validation had no test at all until it moved here." When it acquired a test is not a fact about the code. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Six blocks converted from past-tense incident to present-tense property. The failure mode stays wherever it is the REASON for the design; what goes is the framing that puts it in the past, where it reads as history rather than as a constraint the next change still has to satisfy. - one stream loop: "There were two ... and every rate-limit inconsistency came out of the drift between them" becomes the standing reason separate loops are not allowed back - the test seams: without them a terminal arm "cannot be caught", rather than a transposed counter having "shipped green" once - day_spent!: without it a capped run "spends" the two reads it has no allowance for, present tense, because that is what would happen again - the midnight crossing: the 23:59/795/205 walkthrough is kept in full — it is the clearest statement of why the day is re-read per iteration — but as what WOULD happen, not what did - the day margin: "respected by arithmetic" is now described as what that approach assumes and enforces, not as a thing that was once true - classify: "It used to be skippable" becomes the condition under which it WOULD be skippable, which is the fact a future caller needs BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Copilot's Suppressed section caught this, on an edit from earlier in this same PR. Correcting "wider by one inhabitant" to "wider by two" left the next sentence still saying "a fifth StopReason arm" — singular, from when there was one extra inhabitant to fold in. Folding BOTH list refusals in would take two arms, not one, so the two halves of the paragraph disagreed about the same change. Now: wrapping rather than two more StopReason arms, and those arms would be in scope where drain_streams! constructs `stopped`. The lesson is the reason to read Suppressed sections at all: this review reported "Comments generated: 0 new" and zero unresolved threads on the same pass that carried the finding. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Six blocks in app.roc, including the first P4 case — a comment whose subject was my own earlier reasoning rather than the code. Deleted as self-justification: - "I had deferred this claiming it 'needs a database read inside the message closure, a different shape'; the read fits here, and the claim was wrong." A reader needs to know the read fits inside the closure. That a previous attempt argued otherwise is not a fact about the program. - "each found by enumerating the list rather than by anything in the code", with three key names and "three consecutive rounds". How the gaps were found does not constrain the next change; that the catch-all must be true of EVERY key reaching it does, and that sentence stays. Converted to the standing requirement: - the client-credential arm: `strava_client_id` must not reach the catch-all, and ordering cannot carry that because such an arm is unreachable below `is_secret`. Previously narrated as which key got which sentence. - `init` goes through Output.out! like every other command, because the skill's "EVERY machine response is a versioned envelope" is only true if the setup step honors it. Previously "init printed its line directly, so it was the ONE command that ignored --json". - the opaque-tag catch and the `value: ""` outlier: both restated as what IS the case rather than what was observed once. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
…365 P3) - project_screen carried "(review caught this exact wart returning one PR after events fixed it)". #364 swept seven of these; this one survived because it sits inside a parenthetical rather than opening a sentence. The convention it documents — a PAST horizon states the current baseline — needs no provenance. - the season screen's "Held under the 100-column budget (it shipped at 126, the CLI's only violator)". The budget is the rule; what the screen measured before it was applied is not. - drain_note's tag argument: recovering the tag from the wire string "would be" a stale-able lookup, not "was" one. It is the reason the signature takes both, and it stays true of any future caller. Verified by word-frequency diff against HEAD that the only tokens removed are the ones named above — twice today a same-line-count rewrite silently dropped a sentence, and a line-count check cannot see that. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Three blocks where the concrete failure is the REASON for the design, so it stays — but as what the code prevents, not what once happened. - lens_score's HR guard: a rowing session storing 18.0 bpm scores "improving (221%)" against sessions near 0.85. Present tense, because it is what would happen again without valid_hr. - rank_ts_sql's clause form: comparing MAX(start_local) as a string lets a malformed timestamp outrank every real one. The caps rule above it (THE GUARD'S DOMAIN MUST EQUAL ITS CONSUMER'S) is the general statement; this is now its instance rather than its history. - the hoisting twin's expects: "which shipped with none" and "only one of them was pinned" become the standing property — the predicate is shared, the direction and key are not, so pinning one helper leaves the other open, and the mutation that proves it is stated as what the suite WOULD accept without these. The word-frequency check caught a duplicated line this time: the caps sentence got re-emitted while the original was still there. Same class as the two dropped sentences earlier — a rewrite that spans a line boundary needs the block read back, not just the line count compared. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Both remaining "do not change this" imperatives rewritten. A comment that argues with an imagined critic assumes the reader will not follow the reasoning, so it substitutes an order for it. State the consequence and the order is unnecessary. - Output's missing-value contract: "Do NOT 'fix' that asymmetry by adding a phantom ftp_used_known" becomes the fact that makes the flag pointless — analyze always binds ftp_used, so a NULL-decoded flag would be all-true and carry no information. - Metrics.is_plain_int: "DO NOT 'simplify' this away by calling from_str directly" becomes what calling it directly does — silently restores an unrecoverable write on a fat-fingered argument. Deliberately NOT swept: the ~50 uses of "deliberately" / "on purpose" elsewhere. Those mark a choice that would otherwise read as an oversight (`synced_at` absent from the inputs hash, the DERIVED family absent from secret_keys), which is a real signal to a reader deciding whether they have found a bug. Removing them would be the rewrite this issue is not. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
- plain_keys: "hand-typing this membership from memory shipped a false one three rounds running" is a fact about how the list was maintained, not about the list. What a reader needs is that a membership typed from memory drifts from the routed branches, and that e2e walks the list to catch exactly that. - is_derived: accepting a derived key "would succeed, print a confirmation, and change nothing" — present tense, since it is the reason the rejection exists rather than a report of an old build. - Plan's status_shown: a session completed on another day "would otherwise render" like one completed on time. Same change. The word-frequency check caught an off-by-one in this commit before it landed: the is_derived doc header was overwritten by a replacement aimed one line lower, and the audit surfaced DERIVES and Accepting as words that had vanished without being targeted. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Three blocks, and one of them is the case where the measurement is the whole argument and has to stay. - structure_mates!: "the same 3x12 threshold shipped under three names in three weeks, while one recurring name held 17 sessions of unlike shapes" is the evidence that a class name proves neither sameness nor difference. Kept in full, in present tense, because it is what justifies grouping on shape. - the export SELECT's trailing '': a both-NULL row "decodes as empty rather than as an error", not "used to decode as an error and now reads empty". The reader needs the current behaviour and that it is deliberate. - keep_scored's survival condition: dropped "(measured against the pre-#96 binary)". Which binary the comparison ran against is a fact about the investigation, not about the rule. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
#32 is "Intermittent SIGABRT in the optimized binary". It was cited at six sites for three unrelated defects, so a reader following it landed on the wrong symptom five times out of six. - Report's annotated Bools called a bare tag serializing as the STRING "True" the "#32-class flag bug". That is encoder behaviour, not a crash; the reference is gone and the sentence says what happens. - four str_concat sites said "#32-class" while also naming roc#10595, which IS the empty-string concat bug. The precise reference stays and the imprecise one goes; one of the four had no roc#10595 and now has it, as does the e2e site. - Csv's citation stays. That one is about the compiler's heap corruption, which is what #32 actually reports. `#32` now appears exactly once in the tree, on the bug it names. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Three blocks about wording choices, each written as a report of what an earlier phrasing did. - the noun/verb split: pluralizing both "gives" the wrong sentence, not "produced" it on "7 of 15 real anchors". The mechanism is the reason; how often it was observed is not enforced anywhere. - the trailing clause: what it WOULD describe (a rest of zero on a first structured session, and a mapping that implies the un-annotated rows are the conforming ones) rather than what it did describe. The spread figures that make the mapping wrong are unchanged. - the uniformity cap: "showing 12 of 21" READS as "the other 9 are older" when they ARE the least regular — present tense, since it is why the wording says "most uniform of 21". BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
A duplicate-detection sweep over comment text found a 10-line rationale block repeated almost verbatim in the power and pace twins — 20 lines making one argument twice, differing only in `cp`/`w_prime` vs `cs`/`d_prime` and which expect is named. I had explicitly decided earlier in this issue to KEEP these separate, on the grounds that two independent guards should each state their own reason. Reading them side by side, that was wrong: the ten lines of reasoning are identical, and only the symbol names differ. Two copies of one argument is the drift surface this phase exists to remove. The power twin keeps the full argument. The pace twin now names what is specific to it — the values are `cs`/`d_prime`, the guard is the "critical_speed refuses a physically meaningless fit" expect — and points at the power gate for the reasoning. The consolidation makes those expects the single source of truth, so they were mutation-proved rather than assumed: dropping `or slope <= 0.0` from hyperbolic_fit fails BOTH of them (Metrics.roc:2820 and :3311, "2 failed"), and the restored tree passes as control. The comment's claim that dropping that clause fails the expect is therefore checked, not inherited. Found by normalizing comment text and counting collisions — a mechanical method the read-every-file passes had missed twice. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
The last past-tense incident narrations in src/, converted to the property each one is really about. - resample_1s_pairs: an out-of-order sample dropped without advancing the anchor LETS one large timestamp swallow the rest — present tense, because that is what the ordering guard prevents, not what it once failed to. - the shared power/pace second: separate implementations DISAGREE about what a second means. The two definitions are the reason there is one implementation; that they once diverged is not. - the form-band expects: samples far from the edges let every comparator survive mutation, so these sit ON the edges. Stated as the rule the fixture satisfies rather than as a defect the fixture used to have. - Sports.roc's header both narrated its own move out of Metrics and then said "the header used to claim it had" — a comment describing its own revision history, which is the one thing the rule in CLAUDE.md names outright. Both gone; the four policies and their non-uniformity stay. - Plan's relabel echo, Plan's erased id, Analyze's global-number forward-fill, and Analyze's empty-CASE guard: same conversion. Two false positives left alone: "the 20-min best power used to derive FTP" and "Used to turn a system offset into minutes" are "employed to", not past tense. Method note, because this commit nearly shipped two silent defects. A same-line-count rewrite dropped an `expect` — deleting a Roc expect leaves the suite green, so nothing would have caught it. The word-audit against HEAD surfaced it as a vanished `Metrics.form_band(-15.0)`, and the file was restored and redone with verified extents. Expect counts are now checked per file (289 -> 289). A separate rewrite duplicated a line, which the word audit CANNOT see, since duplication removes nothing — that needs its own adjacent-duplicate check, now also run across every file touched today. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
- ReportHealth's ALL TIME total: "(measured: 475 sessions became 474 at exit 0)" was both past-tense narration and two dataset numbers nothing enforces — they change on the next sync. The property is that an unreadable date silently drops one session from a total printed under ALL TIME, which is why this path refuses rather than reports. - Csv's reverse: appending per byte IS quadratic on real exports, not "became" quadratic in an implementation that no longer exists. - Config's exponent refusal: `config set hr_z1_max 1.18e2` succeeds, echoes, and then makes `summary` report missing_config — present tense, since that is what validating at the WRITE prevents. - Drain's daily cap: "respected by arithmetic" is stated as an approach that does not hold, rather than as a thing the cap used to be. Audited per file: expect counts unchanged (2/4/130/29), no adjacent duplicate lines, and the only words removed are the ones named above. A dropped "against a" was caught by reading the block back at full width rather than through a truncating `cut` — an earlier truncated read this session hid a doubled ", so" and made me "fix" something that was already correct. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
Each of these described what a construct DID once, where what matters is what it does now or would do if changed. - app.roc's platform imports: the alpha4 type-check wall is the reason they are here, stated as the reason rather than as a period the module went through. - Report's `.ok_or(0)`: collapses an unreadable day to epoch 0, rendering a 1969 week row and anchoring the verdict on it. Present tense — it is what the guard prevents, not a bug that was fixed. - ReportSessions' `.ok_or(0)`: same construct, same conversion. - Strava's three sync outcomes: collapsing Unreadable into Absent WOULD force a full re-pull every run. That is why there are three outcomes, and it stays true of any future fourth. Audited per file: expect counts unchanged, no adjacent duplicates, and only the tense words removed. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
The last conversions, plus the two `DO NOT` notes that had to keep their force while losing the incident framing. - Strava's binding copies (two sites): "tried against #105 and crashed real sync every run" becomes "wrapping these in `${...}` to force a copy CRASHES real sync every run". The warning is stronger as a property — it is true of the next person who tries it, which is exactly who the note is for. Both keep the reason they survive #105's fix: the copy is a tempting fix for any future mystery crash. - Db's BadOffset: coalescing an unreadable offset to 0 MAKES it indistinguishable from real UTC. Present tense, because that is what carrying the stored text prevents. - ReportSessions' unscorable banner: the rejected wording is described as what it WOULD be — false wherever unscorable rows render, sitting above a table containing the row it denies — rather than as wording that went stale on a particular day. - app.roc's empty-value refusal and Output's exit-status note: same. Left deliberately: app.roc's `config set <key> ""` comment, which already states the rule that restating deleted behaviour above its replacement tells the reader the opposite of what runs. That is a rule about writing comments, not an incident, and it is the reason the block around it is short. Also left: five grep hits that are not past tense at all — "the 20-min best power used to derive FTP" and "Used to turn a system offset into minutes" mean "employed to", and three "a collapsed row" phrases are present-tense descriptions of current behaviour. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
The last four.
- max_real_gap: folding the gap marker over what SURVIVED would merge a
dropped ride's two neighbouring intervals and announce a break the
athlete never took. That is why the fold runs over real sessions, and
it stays true of any future filter.
- Plan's row_limit: interpolating a clause splices an empty string into
the query and crashes the backend — present tense, because it is the
reason the value stays bound.
- ReportHealth's unrankable line: the undateable line it WOULD otherwise
borrow the repair pointer from is suppressed at zero, which is why this
one has to be self-contained.
P3 is done. What remains under the search pattern is twelve hits, none of
them incidents: five are not past tense at all ("used to derive FTP" and
"Used to turn a system offset" mean "employed to"; three "a collapsed
row" phrases describe current behaviour), and three document the
`config set <key> ""` retirement, which a reader may still attempt and
which #276 is the answer to. Those are facts about what the program does
with an input someone will type.
BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
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.
Counts and referents: - Drain said "both payload sites in Strava.sync!"; there are three, and this PR had already corrected the identical claim in Render's sibling from "Both producer sites" to "All three". Drain was the last holdout. - ReportHealth said two helpers "are here only by adjacency" while its own opening sentence establishes "here" as ReportHealth and both helpers live in Report.roc. One word inverted the claim. - ReportSessions promised three queries and enumerated two, and mis-described the second: the no-LIMIT hint count applies the predicate to the column, not to an alias. Only the capped count uses the alias. - Db's "the pace twin" was left definite with no antecedent after the power original was edited out of the sentence. Named outright now. Head/tail contradictions, both created by appending a correction to a sentence instead of rewriting it: - ReportHealth's km field was widened to "km and hours" while the tail stayed singular — "a payload field", "the one exception", "it stays km". `hours` is a second departure from SI, so "the one exception" was false on its own terms. - Metrics claimed "every sport maps to its own config key" and then, four lines on, that `Config.is_derived` refuses every `ftp_<sport>` key. The head described a world the tail forbids. Leads with the refusal now. - Config's exponent trap was converted to present tense, but the write guard this very comment introduces refuses that input, and its own next sentence says so. Restated as what refusing at the READ sites ALONE would mean. Two blocks P3 had missed, both still pure incident narration by the rule this issue is enforcing: app.roc's "All of these once landed in internal_error" and Output's "The remedy itself also failed once ... Fixed in Analyze.roc". Both now state the property the split and the branch-clearing preserve. BUILD rc=0, SUITE rc=0 (1111 == 1111), four gates rc=0.
There was a problem hiding this comment.
🟢 Approval recommended
Changes are confined to comment/documentation accuracy (no functional modifications) and the updated claims I spot-checked match the referenced code paths.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
eschizoid
force-pushed
the
fix/365-p2-claims
branch
from
September 6, 2026 04:38
99f20df to
7da8beb
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are comment/documentation-only and the updated claims I spot-checked (e.g., schema wording, skill-shapes pin counts, and call-site counts) match the codebase state.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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.
P2 of #365 — claims measured against the code, not taken from the audit. Each was verified before being touched, and several audit findings were rejected rather than fixed.
Corrected
Metrics: swimming/soccer/paddleboard "just work once configured"Config.is_derivedrefuses everyftp_<sport>key. They score via the HR fallback — which is the point the sentence was reaching forAnalyze: skipping a per-key read because "that path corrupts the heap"Err(NoRowsReturned)and would fail on the first missing keyConfig: "whoever wires upmodel_<sport>MUST add it here"ReportHealth: header states helpers live in Report.roc only when shared, names threesport_exact_sqlandsports_with_speed!serve pace-curve alone — my code from #360, with the rule left claiming otherwiseReportHealth: header lists "doctor, stats, zones, power-curve, tte"pace_curve!has lived there since #360 and appeared in neither the list nor the proseStrava: "the tail of a 138-character message"A correctness problem described as a memory-safety one is the costly kind — it sends the next reader hunting the wrong class of bug.
One self-inflicted defect worth noting
My first fix for the
Analyzeclaim restated the real reason inline. That duplicates a rationale the file already carries, which is exactly the defect P5 of this ticket exists for. It now references the existing explanation instead.Verification note
These are the first #365 changes measured under the toolchain the repo actually pins (
nightly-2026-09-04-c125b82). Everything before this was measured againstnightly-2026-08-27because the pin moved mid-branch and I did not notice — comments-only, so nothing broke, but the greens were against the wrong compiler. Re-verified since.Build rc=0, four gates rc=0, suite 1111 == 1111.