NOOP import: recover the band step counter after schema drift (#160)

[abdulsaheel]

User description

Investigating #160 turned up three separate problems. This PR fixes one of them, and documents the other two rather than silently leaving them implied.

1. The reported crash is a .noopbak, not a CSV — NOT fixed here

.noopbak is a ZIP (wrapping SQLite). A ZIP local file header puts the DOS mod-time low byte at offset 10, which is ≥0x80 in most files. Reproduced byte-exactly:

50 4b 03 04 14 00 00 00 08 00 fc 8d 02 5d 29 ba
byte@10 = 0xfc → "can't decode byte 0xfc in position 10"

That is character-for-character the FormatException: Invalid UTF-8 byte (at offset 10) in the issue. import_screen.dart picks FileType.any and noop_import.dart pipes it straight into utf8.decoder, so any non-text file becomes a raw decoder exception. @MrUphill diagnosed this correctly in the thread.

Deliberately out of scope here — it wants a magic-byte sniff plus a human-readable error (and arguably real .noopbak support), which is a separate change from the data fix below.

2. The schema drift — what this PR fixes

NOOP's current export (9.1/9.2):

unix_s,iso_utc,stream,hr_bpm,rr_ms,grav_x,grav_y,grav_z,step_counter,ppg_bpm,ppg_conf,
spo2_red,spo2_ir,skintemp_raw,resp_raw,band_sleep_state,event_kind,event_payload

• band_sleep_state was inserted at index 15, shifting event_kind/event_payload to 16/17. Reading by NAME meant nothing misparsed — the existing defensive design held.
• New streams: steps, band_sleep_state, ppghr.
• spo2 and resp rows are no longer emitted at all, though their columns survive in the header.

The real loss was steps landing in the importer's default: branch. Every imported day reported 0 steps while the band's own counter had measured them — 2,572 over the 3.5 h in the export attached to the issue.

The fix

The cumulative counter is differenced into contiguous runs and banked into live_coverage — the same table the live 100 Hz pedometer writes. That seam needs no change to Substrate or the derivation engine: liveStepsForDay picks up the real count, and coverageWindowsOverlapping keeps the 1 Hz estimate from double-counting those minutes. Imported and live days are counted identically.

stepRuns is pure and unit-tested:

• splits on gaps > 60 s, so the export's 20.5 h hole never becomes one window claiming to cover the day
• sums only positive deltas — a negative delta is a reboot reset, not −24,000 steps
• drops deltas across a run boundary rather than attributing steps to a window we have no samples for
• emits nothing for a 0-step run, which would suppress a real 1 Hz estimate while contributing nothing

_flushStepCoverage is idempotent via hasLiveCoverageWindow: live_coverage is an append-only SUM with no uniqueness on the window, so re-importing the same export would otherwise double every step.

Verified against the real file from the issue:

STEPS=2572   coverage 1785480900..1785493571
"steps":{"value":2572,"real_100hz":2572,"tier":"HIGH"}     ← was steps=0.0

2,572 matches the counter range (24302 → 26874) measured independently, and the window covers only the 3.5 h block, not the hole.

Deliberately not consumed

stream	measured	why skipped
band_sleep_state	constant 0 across all 12,663 rows	no signal, and a second sleep source would contradict segmentSleep
ppghr	151 samples, conf 0.28–0.81	hr already covers all 12,692 seconds; PPG-derived HR isn't trusted as a substitute

Both are documented in the file header with the reasoning, so the next reader doesn't have to re-derive it.

3. Why the data still looks thin — no code change

Worth stating plainly for the reporters: the CSV was importing fine all along (days=2 rows=66210 lateRows=0). It just doesn't contain much:

07-31T06  8.3% · T07 100% · T08 100% · T09 99.9% · T10 43.6% · [gap 73672 s = 20.5 h] · 08-01T06 0.8%

One 3.5 h daytime block, then a 20.5 h hole. No overnight ⇒ no sleep ⇒ RMSSD/readiness/strain are legitimately null. That matches @Jenssaibe's "it is only the last 24 hours" — a NOOP export-scope limitation, not an edge bug. No warning UI added; the import reports honestly and the metrics abstain rather than fabricate.

Version

kAlgoVersion 53 → 54. Only the steps/active_min block of imported days changes; no live-sync output moves.

⚠️ The bump does not retro-fix an existing import — imported days are force-finalized snapshots with no stored raw to recompute from, so an already-imported day needs a re-import to pick its steps up. Called out in the changelog comment.

Tests

test/noop_schema_drift_test.dart — 11 tests. Seven pure stepRuns cases (counter reset, gap split, cross-boundary non-attribution, zero-step run, order independence) and four end-to-end against the current header, including re-import idempotency, an export with no steps stream, and an unknown future stream.

There was previously no test that parsed a real NOOP CSV at all — only the pure decideRow ordering contract. That is precisely why this drift shipped unnoticed, and the end-to-end cases are the part that would have caught it.

Full suite: 1099 passing, flutter analyze clean.

Note for reviewers

One cosmetic honesty gap left alone: an imported day's steps block reports inputs_used: ["live_100hz_pedometer", ...] and the note "real 100 Hz count only". Accurate as "the real-count channel", inaccurate as provenance for a NOOP import. Fixing it means threading a source flag through _stepsAndEnergy/_DeriveInput, which felt like a poor trade against touching the shared derive path in this PR. Happy to do it if you'd rather.

🤖 Generated with Claude Code

---

PR Type

Bug fix, Tests

---

Description

• NOOP CSV importer now recovers real step counts from the step_counter column instead of silently dropping them (2,572+ steps lost per import in issue Encoding error when important data from NOOP #160).

• stepRuns() differences the cumulative counter into contiguous windows, splits on gaps >60 s, ignores negative deltas (reboots), and banks results into live_coverage — idempotent via hasLiveCoverageWindow.

• kAlgoVersion bumped from 53 → 54; only imported days' steps/active_min block changes; no live-sync output moves.

• New regression test file pins the schema-drift fix, stepRuns pure logic, idempotent re-import, and graceful handling of unknown future streams.

---

Diagram Walkthrough

flowchart LR
  A["NOOP CSV\n(steps stream)"] -- "parse step_counter\nby column NAME" --> B["stepRuns()\npure function"]
  B -- "split on gaps >60s\nsum positive deltas" --> C["StepRun list"]
  C -- "_flushStepCoverage()\nidempotent via hasLiveCoverageWindow" --> D["live_coverage\ntable (LocalDb)"]
  D -- "liveStepsForDay()\ncoverageWindowsOverlapping()" --> E["DerivationEngine\n(kAlgoVersion=54)"]
  E --> F["steps / active_min\nin day_result"]

Loading

File Walkthrough

	Relevant files
Bug fix	derivation_engine.dart Bump kAlgoVersion to 54 for NOOP step recovery                      lib/compute/derivation_engine.dart Bumps kAlgoVersion from 53 to 54. Adds changelog entry explaining that only imported days' steps/active_min block changes; no live-sync output moves. Notes that already-imported days need a re-import to pick up real steps (no stored raw to recompute from). +14/-1    noop_import.dart Recover real step counts from NOOP step_counter stream      lib/import/noop_import.dart Updates file header to document the current NOOP 9.1/9.2 schema (new band_sleep_state column at index 15, new streams steps/band_sleep_state/ppghr, absent spo2/resp rows) and explains what is consumed vs. deliberately ignored. Adds StepRun value class and stepRuns() pure static method that differences the cumulative step_counter into contiguous windows, splits on gaps >60 s, ignores negative deltas (counter resets), and emits nothing for zero-step runs. Adds _flushStepCoverage() which banks StepRuns into live_coverage idempotently via hasLiveCoverageWindow, returning the steps actually banked. Wires step accumulation into the main parse loop (per-date stepsByDate map), flushes before each deriveAndPrune call, and handles the EOF and steps-only-date edge cases; adds steps field to NoopImportResult. +150/-9
Tests	noop_schema_drift_test.dart Add regression tests for NOOP schema drift and step recovery test/noop_schema_drift_test.dart New regression test file (213 lines) covering the NOOP schema-drift fix. Pure unit tests for stepRuns(): positive deltas, gap splitting, counter reset, cross-boundary attribution, zero-step run, empty/single-sample input, and order-independence. End-to-end integration tests using sqflite_ffi: verifies steps are banked correctly, re-import is idempotent (no double-counting), shifted event_kind/event_payload columns do not misparse, missing steps stream imports cleanly with 0 steps, and unknown future streams are skipped without error. +213/-0

---

Summary by CodeRabbit

• New Features

  • CSV imports now preserve real step counts from supported step-counter data.
  • Imported step data is applied consistently with live activity data without double-counting estimates.
  • Imports now handle current schema variations, quoted events, missing streams, and unknown streams more reliably.

• Bug Fixes

  • Corrected overlapping or repeated imports to prevent duplicate step coverage.
  • Improved handling of counter resets, gaps, and incomplete import dates.

[coderabbitai]

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1ca4340b-8206-41c8-bdb4-abf12aa65df7

📥 Commits

Reviewing files that changed from the base of the PR and between 533ada3 and b0d3526.

📒 Files selected for processing (1)

• test/noop_schema_drift_test.dart

📝 Walkthrough

Walkthrough

The NOOP importer now reads cumulative step counters, converts valid counter runs into real-step coverage, and persists them idempotently before derivation. Schema handling and regression tests cover reordered columns, missing streams, unknown streams, resets, gaps, and repeated imports.

Changes

NOOP real-step import

Layer / File(s)	Summary
Parse and buffer step-counter data lib/import/noop_import.dart	The importer documents the current NOOP schema, parses steps rows by stream name, exposes recovered step totals, and buffers samples by date.
Segment and persist real-step runs lib/import/noop_import.dart, lib/compute/derivation_engine.dart	Cumulative counters become positive contiguous runs. New runs are stored in live_coverage without duplicate persistence. kAlgoVersion increases to 54.
Validate schema and import behavior test/noop_schema_drift_test.dart	Tests cover run segmentation, schema drift, real-step storage, idempotent imports, missing step streams, and unknown streams.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NOOPCSV
  participant NoopImporter
  participant live_coverage
  participant DerivationEngine
  NOOPCSV->>NoopImporter: Provide steps rows
  NoopImporter->>NoopImporter: Build contiguous StepRun values
  NoopImporter->>live_coverage: Persist new step windows
  NoopImporter->>DerivationEngine: Derive each imported date

Loading

Possibly related PRs

• OpenStrap/edge#158: Extends the same NoopImporter and step-processing work.
• OpenStrap/edge#172: Changes related live-coverage and step-derivation behavior in lib/compute/derivation_engine.dart.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: recovering band step-counter data during NOOP imports after schema drift.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b0d3526)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Non-atomic flush _flushStepCoverage is called per-date with individual await LocalDb.addLiveCoverage(...) calls inside a loop, with no wrapping transaction. If the process is killed between two addLiveCoverage writes for the same date, the partial state is undetectable: the covered-clipping logic will see the already-written windows and skip the missing ones on re-import only if the re-import produces the same run boundaries. The test for "partially flushed import self-heals" simulates a crash between two dates (two separate _flushStepCoverage calls), not between two runs within one date. A crash mid-loop within a single date's flush leaves a partial run set that the covered-clipping logic will correctly recover from only if the run boundaries are stable — which they are, since samples are keyed by timestamp. This is a lower-severity concern given the clipping logic, but the comment "this is why the flush does not need to be transactional" in the test overstates the safety: it holds for the between-dates case but not for within-date partial writes where a run is split by the covered check into sub-runs that are written sequentially. static Future<int> _flushStepCoverage( Map<int, int> stepSamples, String date) async { if (stepSamples.isEmpty) return 0; final ts = stepSamples.keys.toList()..sort(); final existing = await LocalDb.coverageWindowsOverlapping(ts.first, ts.last + 1); var banked = 0; for (final r in stepRuns(stepSamples, covered: existing)) { await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); banked += r.steps; } return banked; } Steps-only date not derived The code at the end of importFile flushes any remaining stepsByDate entries that were never derived (dates that carried ONLY a steps stream). However, _flushStepCoverage is static and takes no LocalDb instance — it calls LocalDb.coverageWindowsOverlapping and LocalDb.addLiveCoverage as static methods. The steps are banked, but engine.deriveImportedDays is never called for these dates, so the live_coverage rows exist but no day_result is produced. The derivation that reads liveStepsForDay never runs for these orphaned dates. In practice a steps-only date is unlikely (the export always includes hr rows), but the comment "a date that carried ONLY a steps stream" explicitly anticipates this case and the handling is incomplete. // Any date whose steps were buffered but which never derived (e.g. a date // that carried ONLY a `steps` stream) still banks its real count — dropping // it would silently lose steps the band actually measured. for (final e in stepsByDate.entries) { stepsBanked += await _flushStepCoverage(e.value, e.key); } stepRuns loop boundary In stepRuns, the loop runs for (var i = 1; i <= ts.length; i++). When i == ts.length, bankable is always false (the i < ts.length guard), so closeRun() is called. This correctly closes the final run. However, runLast is set to ts[i] inside the loop body only when bankable is true and i < ts.length, so the final closeRun() uses whatever runLast was set to on the previous iteration. This is correct by design but the loop reads ts[i] (which would be out-of-bounds) only when bankable is true, and bankable requires i < ts.length, so there is no actual out-of-bounds access. The logic is correct but the loop bound i <= ts.length is non-idiomatic and warrants a careful read to confirm safety — confirmed safe here, but a future maintainer editing the bankable condition could introduce an OOB. for (var i = 1; i <= ts.length; i++) { final bankable = i < ts.length && ts[i] - ts[i - 1] <= stepRunMaxGapSec && !isCovered(ts[i - 1], ts[i]); if (!bankable) { closeRun(); continue; } runStart ??= ts[i - 1]; final d = samples[ts[i]]! - samples[ts[i - 1]]!; if (d > 0) runSteps += d; runLast = ts[i]; } return out;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b0d3526

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix lost run anchor after a covered delta When i == ts.length the loop body always calls closeRun() and continues, which is correct for flushing the final run. However when i < ts.length and isCovered is true, closeRun() is called but the current sample at ts[i] is silently skipped as a potential run start — the next iteration begins at ts[i+1]. This means a covered delta at position i also discards the sample at ts[i] as a new run anchor, so steps immediately after a covered window are lost. The run should only be closed; ts[i] should remain eligible as the start of the next run. lib/import/noop_import.dart [399-411] for (var i = 1; i <= ts.length; i++) { - final bankable = i < ts.length && - ts[i] - ts[i - 1] <= stepRunMaxGapSec && - !isCovered(ts[i - 1], ts[i]); - if (!bankable) { + if (i == ts.length) { closeRun(); + break; + } + final gap = ts[i] - ts[i - 1] > stepRunMaxGapSec; + final covered = isCovered(ts[i - 1], ts[i]); + if (gap || covered) { + closeRun(); + // ts[i] is still a valid anchor for the next run. continue; } runStart ??= ts[i - 1]; final d = samples[ts[i]]! - samples[ts[i - 1]]!; if (d > 0) runSteps += d; runLast = ts[i]; } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a real behavioral difference: when a delta is covered, the current code calls closeRun() and continues, which means ts[i] is skipped as a potential run anchor for the next iteration. The improved code correctly separates gap and covered checks and preserves ts[i] as a valid next-run start. However, the existing test 'skips deltas already covered, and breaks the run there' passes with the current code, suggesting the practical impact may be limited to edge cases not covered by tests.	Low
	Batch coverage writes to prevent double-count race Each addLiveCoverage call is awaited individually inside the loop, so a crash between two runs leaves live_coverage partially written. The test for "partially flushed import self-heals" relies on this being recoverable, but the real risk is that coverageWindowsOverlapping is read once before the loop: if a second concurrent flush for the same date races in (e.g. the EOF flush and the stepsByDate residual loop both fire for the same date), the pre-read existing snapshot is stale and both flushes see the same uncovered windows, doubling the count. The runs should be written in a single batch transaction so the read-then-write is atomic. lib/import/noop_import.dart [429-441] static Future<int> _flushStepCoverage( Map<int, int> stepSamples, String date) async { if (stepSamples.isEmpty) return 0; final ts = stepSamples.keys.toList()..sort(); final existing = await LocalDb.coverageWindowsOverlapping(ts.first, ts.last + 1); + final runs = stepRuns(stepSamples, covered: existing); + if (runs.isEmpty) return 0; var banked = 0; - for (final r in stepRuns(stepSamples, covered: existing)) { - await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); - banked += r.steps; - } + await LocalDb.addLiveCoverageAll( + runs.map((r) => (r.startSec, r.endSec, r.steps, date)).toList(), + ); + for (final r in runs) banked += r.steps; return banked; } Suggestion importance[1-10]: 3 __ Why: The race condition described requires two concurrent flushes for the same date, but the importer is sequential (single async function) and stepsByDate.remove(date) ensures each date's steps are flushed exactly once. The suggestion introduces a new LocalDb.addLiveCoverageAll API that doesn't exist in the PR, making the improved_code non-applicable as-is. The early-return optimization for empty runs is valid but minor.	Low

---

Previous suggestions

Suggestions up to commit 533ada3

Category	Suggestion	Impact
Possible issue	Make step coverage flush atomic via a transaction _flushStepCoverage calls LocalDb.addLiveCoverage once per StepRun in separate awaits with no enclosing transaction. If the process is interrupted mid-flush, some runs are banked and others are not, leaving live_coverage in a partially-written state that the covered-clipping logic will then treat as already banked — silently losing the unwritten steps on any subsequent re-import. All addLiveCoverage inserts for a given date should be wrapped in a single database transaction so the flush is atomic. lib/import/noop_import.dart [429-441] static Future<int> _flushStepCoverage( Map<int, int> stepSamples, String date) async { if (stepSamples.isEmpty) return 0; final ts = stepSamples.keys.toList()..sort(); final existing = await LocalDb.coverageWindowsOverlapping(ts.first, ts.last + 1); + final runs = stepRuns(stepSamples, covered: existing); + if (runs.isEmpty) return 0; var banked = 0; - for (final r in stepRuns(stepSamples, covered: existing)) { - await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); - banked += r.steps; - } + await LocalDb.inTransaction(() async { + for (final r in runs) { + await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); + banked += r.steps; + } + }); return banked; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that multiple addLiveCoverage inserts in _flushStepCoverage are not atomic, which could leave live_coverage in a partial state on interruption. However, this requires LocalDb.inTransaction to exist as an API, and the impact is limited since partial flushes would still be partially protected by the covered-clipping logic on re-import.	Low
General	Skip orphaned step coverage for already-pruned dates This loop iterates over stepsByDate after the EOF block has already called stepsByDate.remove(curDate) for the final buffered date. However, any date that was in derived (already derived and pruned) but still had step samples buffered would also remain in stepsByDate here and get flushed — which is correct. But dates that were processed through deriveAndPrune already had their entry removed via stepsByDate.remove(date). The real risk is that stepsByDate may still contain entries for dates that were skipped as lateRows (rows whose date was already in derived). For those dates the derivation never ran, so live_coverage rows would be written for a date with no corresponding day_result, causing the derivation to never read them. These orphaned coverage rows should be skipped rather than banked. lib/import/noop_import.dart [265-270] // Any date whose steps were buffered but which never derived (e.g. a date // that carried ONLY a `steps` stream) still banks its real count — dropping // it would silently lose steps the band actually measured. +// Skip dates that were already derived+pruned before their steps arrived +// (lateRow dates): those have no day_result to read the coverage. for (final e in stepsByDate.entries) { + if (derived.contains(e.key)) continue; stepsBanked += await _flushStepCoverage(e.value, e.key); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about lateRow dates remaining in stepsByDate and getting their coverage banked without a corresponding derivation. However, the derived set tracks dates already derived+pruned, and late-row dates would indeed be in derived, making the if (derived.contains(e.key)) continue guard a reasonable safeguard. The improved code accurately reflects the suggested change.	Low

Suggestions up to commit c16f992

Category	Suggestion	Impact
Possible issue	Flush orphan-date steps before finalization Steps for dates that carried only a steps stream (no HR/gravity rows) are flushed here after engine.finalizeImport has already been called. Because _flushStepCoverage writes to live_coverage after finalization, those steps will never be read by liveStepsForDay during derivation — they are banked too late to affect the derived scalar. These orphan-date steps should be flushed before finalizeImport, or the comment should acknowledge that they are stored for future re-derives only. lib/import/noop_import.dart [268-270] for (final e in stepsByDate.entries) { stepsBanked += await _flushStepCoverage(e.value, e.key); } +await engine.finalizeImport(profile); + Suggestion importance[1-10]: 6 __ Why: The stepsByDate entries for dates with only a steps stream are flushed after engine.finalizeImport is called, meaning those steps are banked too late to be read by liveStepsForDay during derivation. The improved_code correctly shows moving finalizeImport after the flush loop, which is a meaningful correctness fix for this edge case.	Low
	Fix TOCTOU race in idempotency guard The idempotency guard uses a check-then-act pattern without a lock: two concurrent re-imports of the same file can both pass hasLiveCoverageWindow before either writes, causing the window to be inserted twice and doubling the step count. Since live_coverage has no uniqueness constraint on the window, the database cannot reject the duplicate. Use an INSERT-OR-IGNORE with a UNIQUE constraint on (start_sec, end_sec) in addLiveCoverage, or wrap the check+insert in a single transaction inside LocalDb so the race cannot occur. lib/import/noop_import.dart [391-400] static Future<int> _flushStepCoverage( Map<int, int> stepSamples, String date) async { var banked = 0; for (final r in stepRuns(stepSamples)) { - if (await LocalDb.hasLiveCoverageWindow(r.startSec, r.endSec)) continue; - await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); - banked += r.steps; + // Use atomic insert-or-ignore so concurrent re-imports cannot + // double-count: the uniqueness is enforced by the DB, not by a + // check-then-act race. + final inserted = + await LocalDb.addLiveCoverageIfAbsent(r.startSec, r.endSec, r.steps, date); + if (inserted) banked += r.steps; } return banked; } Suggestion importance[1-10]: 5 __ Why: The check-then-act pattern in _flushStepCoverage could theoretically allow duplicate inserts in concurrent re-imports, but in practice this importer runs sequentially and the race window is extremely narrow. The suggestion is valid but the impact is low given the sequential nature of the import process.	Low

[abdulsaheel]

Went through every bot finding against the actual code rather than taking them at face value. One was real and is now fixed in 533ada3; three were false; one is acknowledged but not worth acting on.

✅ Real — and worse than described (533ada3)

"Idempotence Broken" (PR Agent)

The stated premise is wrong: hasLiveCoverageWindow is an exact match, not the range check it hypothesised —

SELECT 1 FROM live_coverage WHERE start_ts = ? AND end_ts = ? LIMIT 1

But the failure it predicted from that wrong premise is real for a different reason, and it reproduced on the first try. An exact-window check only defends a byte-identical re-import. The realistic case walks straight past it: export again later over a longer span covering the same session.

import #1 (20 min):              steps=1199
import #2 (40 min, OVERLAPS #1): steps=2399
window 1785488400..1785489599 steps=1199
window 1785488400..1785490799 steps=2399
TOTAL banked = 3598   (TRUTH = 2399)   ← 50% inflated

The run boundary moves, no exact window matches, the whole overlap is banked twice. Exporting again next week is a completely ordinary thing to do, so this would have hit real users.

Fix: stepRuns now takes the spans already in live_coverage and skips any per-second delta intersecting one, breaking the run there. Clipping is exact, not pro-rated — we hold the counter at every second, so uncovered sub-intervals are summed from real deltas. The exact-window check is dropped, since covered-clipping strictly subsumes it. It also stops an imported span double-counting against a live 100 Hz pedometer window, which shares this table.

import #2 (40 min, OVERLAPS #1): steps=1200
window 1785488400..1785489599 steps=1199
window 1785489599..1785490799 steps=1200
TOTAL banked = 2399   ← truth

Real #160 export re-verified: still 2,572 on first import, 0 on re-import, one window. Four new tests (three pure, one end-to-end) pin 2,399 rather than 3,598.

❌ Not real — verified against code

"Missing Absent-Input Guard" (PR Agent) — hedged on whether at() can throw. It cannot:

String at(List<String> f, String name) {
  final i = idx(name);
  return (i != null && i < f.length) ? f[i] : '';
}

Returns '' on any miss. A future schema dropping step_counter yields int.tryParse('') == null and the sample is skipped — the behaviour the finding asks for is already there.

"Flush orphan-date steps before finalization" (PR Agent suggestion) — the suggested diff adds await engine.finalizeImport(profile); after the flush loop. That is already the order:

269:  stepsBanked += await _flushStepCoverage(e.value, e.key);   ← flush
272:  await engine.finalizeImport(profile);                       ← then finalize

Misread of the diff; no change needed.

"Steps Lost on Derive-Only Date" (PR Agent) — the premise is right (a derived date is already removed, so the loop only sees dates that never derived) but the conclusion doesn't follow. That loop is a deliberate backstop for a date buffered out-of-order or one where secs is empty at EOF. Those steps are banked to live_coverage and picked up whenever that day is derived — dropping them instead would be the actual data loss. Not "misleading": NoopImportResult.steps is documented as steps banked, not steps derived.

⚠️ Acknowledged, not acting

TOCTOU race on the idempotency guard (PR Agent suggestion, self-rated Low) — correct in principle. In practice the importer is strictly sequential and gated by _busy/_picking in import_screen.dart, so there is no concurrent path to race. Worth revisiting only if imports ever go parallel; the covered-clipping change also shrinks the window, since the check now reads spans rather than one exact key. Adding a UNIQUE(start_ts, end_ts) constraint would need a migration on a table the live pedometer writes to, which is a bigger change than the risk justifies here.

---

CodeRabbit was still processing when I wrote this — happy to do another pass once it lands.

Full suite after the fix: 1103 passing, flutter analyze clean.

[github-actions]

Persistent review updated to latest commit 533ada3

[abdulsaheel]

Follow-up on the post-533ada3 suggestion. Checked it; it's false on both counts — but it pointed at a property worth pinning, so b0d3526 adds a test for it.

❌ "Make step coverage flush atomic via a transaction"

The suggested API doesn't exist. LocalDb.inTransaction isn't a thing in this codebase — grep returns nothing. The pattern here is inline db.transaction((txn) async {...}).

The failure mode doesn't happen either. The claim is that an interrupted flush leaves live_coverage partially written, and covered-clipping then "silently loses the unwritten steps on any subsequent re-import."

That inverts how the clipping works. Runs are disjoint time spans. A run that never landed is, by definition, not covered — so the next import banks it. Tested by writing a two-run import, deleting the second row to simulate a crash between runs, then re-importing:

full import:                       steps=1198
  window 1785488400..1785488999 steps=599
  window 1785489600..1785490199 steps=599
simulated partial flush: deleted run B (599 steps)
re-import after partial flush:     steps=599     ← exactly the lost run
windows=2 total=1198 (truth 1198)  ← RECOVERED

Not just "not lost" — recovered exactly, with no double-count, because the surviving run is still covered and gets clipped.

This is a direct consequence of the 533ada3 change. Keying idempotency by time span instead of by an exact window row is what makes the flush recoverable rather than atomic — which is a strictly better property than the transaction would have bought, since a transaction protects against a torn write but still leaves you with nothing banked, whereas this recovers on the next import either way.

What I did take from it

The suggestion was wrong, but "what happens if the flush is interrupted?" is a fair question that the code only answered implicitly. b0d3526 makes it explicit with a regression test, so nobody has to re-derive it — and so a future change that swaps time-span clipping back to an exact-window key fails loudly instead of silently reintroducing the hazard.

---

Scoreboard across both bot passes: 1 real finding (the overlap double-count — genuinely valuable, would have hit users), 4 false, 1 acknowledged-but-not-worth-acting-on. The real one is fixed and the false ones are documented above rather than silently dropped.

17 tests in noop_schema_drift_test.dart, full suite 1104 passing, flutter analyze clean.

[github-actions]

Persistent review updated to latest commit b0d3526