Refactor and harden the Go implementation

[richhaase]

Summary

• Extract the report renderer from the CLI leaf into a directly testable internal/report package and add a small internal/syncer orchestration layer.
• Make whole-file writes crash-safe with same-directory temporary files, syncing, and atomic replacement; strengthen store-copy verification with SHA-256 and symlink rejection.
• Correct Concept2 incremental sync semantics by using updated_after, upserting edited workouts, retrying every missing or corrupt stroke file, and advancing a pre-fetch high-water mark only after durable workout storage.
• Tighten goal/date calculations, schema checks, operational error propagation, ULID generation, terminal token entry, CSV encoding, HTTPS API validation, and Windows report opening.
• Expand focused tests across API, config, storage, sync, reports, documents, paths, envelopes, CSV, dates, and failure cases.
• Pin vulnerability tooling, require patched Go 1.26.5, and gate releases on the repository checks and vulnerability scan.

Why

The review found a handful of correctness and durability gaps hidden behind otherwise clean code: incremental sync used the workout-date filter for a timestamp, edited workouts were never updated locally, failed stroke downloads could be skipped indefinitely, several file replacements were vulnerable to truncation, and date arithmetic was duration-based across DST boundaries. The refactor keeps the existing architecture and data contracts while making those boundaries explicit and testable.

The sync behavior follows the official Concept2 API contract: from filters workout dates, while updated_after filters creation and update timestamps. See https://log.concept2.com/developers/documentation/.

Impact

Existing commands and machine-readable envelope schemas remain intact. Users gain durable file updates, correct incremental reconciliation, retryable stroke synchronization, stricter foreign/unsupported-store protection, hidden token entry on terminals, standards-compliant CSV output, and clearer failures when filesystem operations cannot be completed.

Validation

• make check
• make vuln
• go mod tidy -diff
• go mod verify
• go test -shuffle=on -count=10 ./...
• GOOS=windows GOARCH=amd64 go build ./...
• GOOS=linux GOARCH=arm64 go build ./...
• actionlint
• goreleaser check
• Aggregate statement coverage: 72.6% (baseline: 53.3%)

[richhaase]

Findings

1. Stroke retries continue after repeated API failures (1/3 reviewers)

Synchronization keeps requesting missing stroke files without a cutoff, so repeated 30-second API failures can make large syncs take hours.

Evidence:

• [P1] Stop stroke retries after repeated API failures — /Users/rdh/src/c2/.worktrees/review-pr37-f8a9b8fd/internal/syncer/syncer.go:124-126

2. Partial final goal weeks remain unelapsed (1/3 reviewers)

WeeksElapsed rounds down even after goals with partial final weeks have ended, producing values such as 0 / 1 or 52 / 53 instead of matching TotalWeeks.

Evidence:

• [P2] Mark partial final weeks elapsed at the goal end — /Users/rdh/src/c2/.worktrees/review-pr37-f8a9b8fd/internal/stats/stats.go:263-265

Expand for verbatim findings.

Raw findings (verbatim)

1. (1/3 reviewers)

The patch introduces an unbounded stroke-retry loop that can make synchronization effectively unusable during API failures, and its inclusive goal-week calculation reports incomplete elapsed periods after some goals have ended.

Full review comments:

- [P1] Stop stroke retries after repeated API failures — /Users/rdh/src/c2/.worktrees/review-pr37-f8a9b8fd/internal/syncer/syncer.go:124-126
  When the stroke endpoint is unavailable and many stored workouts lack stroke files, every failed request is followed by another request for the remaining workouts. Each request can consume the 30-second client timeout, so a sync with hundreds of workouts can take hours; `strokeFailureLimit` now suppresses only messages, unlike the previous implementation's request cutoff. Stop or back off after repeated failures while leaving the remaining workouts for the next sync.

- [P2] Mark partial final weeks elapsed at the goal end — /Users/rdh/src/c2/.worktrees/review-pr37-f8a9b8fd/internal/stats/stats.go:263-265
  When the inclusive goal span is not an exact multiple of seven days, `TotalWeeks` is rounded up while `WeeksElapsed` is rounded down. Consequently, after the goal has ended a one-day goal still reports `0 / 1` weeks elapsed, and a 365-day goal reports `52 / 53`, until the next full-week boundary. Clamp `WeeksElapsed` to `TotalWeeks` once `today` reaches `endExclusive`.

Posted by acr 0.20.4

[richhaase]

Findings

1. Failed strokes starve later downloads (3/3 reviewers)

The fixed retry cap repeatedly stops on the same failed workouts, permanently preventing later missing stroke files from being attempted.

Evidence:

• [P1] Prevent failed strokes from starving the rest — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129
• [P1] Prevent failed strokes from starving later workouts — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129
• [P1] Avoid starving later stroke downloads after repeated failures — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129

2. Setup can save an invalid goal range (1/3 reviewers)

Keeping the existing end date after moving the start beyond it saves an inverted range, causing goal-related commands to fail despite setup reporting success.

Evidence:

• [P2] Keep the goal range valid when only the start changes — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/cli/setup.go:209-210

3. Missing home directory permits unintended initialization (1/3 reviewers)

Discarding the home-directory lookup error produces an empty default data path, allowing setup to initialize the current directory before configuration saving fails.

Evidence:

• [P2] Propagate the missing-home error from defaults — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/config/config.go:64-64

Expand for verbatim findings.

Raw findings (verbatim)

1. (1/3 reviewers)

The new stroke retry cap can permanently prevent later workouts from receiving stroke data when the same earlier records repeatedly fail. Other inspected changes and available checks passed.

Review comment:

- [P1] Prevent failed strokes from starving the rest — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129
  When three earlier workouts consistently return an error or no samples, every `c2 sync` retries those same workouts in the unchanged JSONL order and breaks here. Consequently, all later workouts with missing stroke files are never attempted, even across subsequent syncs; the failure cap needs to rotate or persist progress rather than always stopping at the same records.

2. (1/3 reviewers)

The stroke retry cap can permanently prevent later workouts from receiving stroke data when the same earlier records repeatedly fail.

Review comment:

- [P1] Prevent failed strokes from starving later workouts — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129
  When three earlier workouts consistently return errors or no samples, every sync retries those same workouts in the stable JSONL order and breaks at this limit. Later workouts with missing stroke files are therefore never attempted, even across subsequent syncs; rotate or persist retry progress so the failure cap still allows every pending workout to be reached eventually.

3. (1/3 reviewers)

The patch can permanently starve valid stroke downloads and allows setup to save an invalid goal range. It can also initialize the current directory when no home directory is available before failing to save configuration.

Full review comments:

- [P1] Avoid starving later stroke downloads after repeated failures — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/syncer/syncer.go:128-129
  When the same first three missing stroke files consistently fail or return no samples, every subsequent sync traverses the same storage order and breaks here again. Any workouts after those records are therefore never attempted, despite the message that missing data will be retried; limit reporting or rotate/persist progress without permanently starving later workouts.

- [P2] Keep the goal range valid when only the start changes — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/cli/setup.go:209-210
  When a user moves the start date past the configured end and presses Enter to retain that end, this branch says it is keeping the previous value—which is exactly the now-invalid value—and then saves the inverted range. `ComputeGoalProgress` now rejects that configuration, so status, goal stats, and reports fail immediately after setup reports success; revert the start or require a valid replacement end instead.

- [P2] Propagate the missing-home error from defaults — /Users/rdh/src/c2/.worktrees/review-pr37-b36eb8a6/internal/config/config.go:64-64
  When `os.UserHomeDir` fails, such as with `HOME` unset, discarding this error leaves `Default().DataDir` empty. Setup catches the load failure and uses that default, so accepting the blank data-directory prompt resolves to the current working directory and initializes a store there before config saving inevitably fails; abort setup before filesystem writes instead of returning an empty default path.

Posted by acr 0.20.4

[richhaase]

Addressed all ACR findings on the current head (76669fb):

• Capped repeated stroke API failures while preserving retry progress (cb4fe83).
• Clamped elapsed goal weeks at the inclusive goal end (cb4fe83).
• Persisted a rotating stroke cursor so repeated failures cannot starve later workouts (76669fb).
• Prevented setup from saving an inverted goal range (76669fb).
• Propagated missing-home errors before setup performs filesystem writes (76669fb).

Validation completed with make check and make vuln; all PR CI checks are passing. Please re-review the current head.

[richhaase]

LGTM ✅

3 of 3 reviewers found no issues.

Reviewer comments

• Reviewer 1: No actionable correctness issues found in the diff. Test execution was constrained by sandbox network/listener restrictions.
  Categorized as informational during summarization
• Reviewer 2: No actionable regressions were identified in the diff. The test suite compiles and passes except API tests that cannot open loopback sockets in the sandbox.
  Categorized as informational during summarization
• Reviewer 3: No actionable correctness issues were found in the changes relative to the specified base commit.
  Categorized as informational during summarization

Posted by acr 0.20.4