Fix recurring completion error masking

openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-07-19

openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/design.md

@@ -0,0 +1,80 @@
+# Design: fix-recurring-completion-error-masking
+
+Context: driven by FR1–FR7 from `proposal.md`.
+
+## Context
+
+`TaskService.complete()` (`packages/client/src/services/TaskService.ts`) parses the repeat rule and, on success, calculates the next date and creates/updates a recurring copy — all inside one `try/catch`. Two distinct failure modes currently collapse into one return value:
+
+1. `parseRepeatRule()` returns `null` → early return `{ status: 'skipped_invalid_rule' }` (the rule genuinely can't be parsed).
+2. Anything *after* a successful parse throws (date math, `findHiddenRecurringTask`, `createRecurringCopy` → `create()` → `generateTopKey()`, checklist copy, a Dexie write) → the broad `catch` returns the **same** `{ status: 'skipped_invalid_rule' }`.
+
+The `RecurringResult` union has three members (`created` | `skipped_invalid_rule` | `not_recurring`). Three live completion callers (`useTasks`, `useInboxTasks`, `useTaskMutations`) map `skipped_invalid_rule` → a `repeat_rule_invalid` alert, which renders the "Rule not recognized / could not be recognized" dialog. So mode (2) tells the user their (valid) rule is broken. A fourth hook, `useTask`, carries the same block but is dead code (no importers outside its own test) — removed by this change, see D5.
+
+The `invalid-repeat-rule-detection` and `repeating-tasks` specs both currently *codify* this conflation ("Exception during copy creation returns skipped status"), so the specs must change too, not just the code.
+
+## Goals / Non-Goals
+
+**Goals:**
+- Give the exception path its own status so callers can distinguish it and not lie to the user.
+- Keep `skipped_invalid_rule` a truthful signal (parse-failure only).
+- Preserve every other observable of completion: the task is still completed, the error is still swallowed and logged.
+
+**Non-Goals:**
+- No new user-facing alert type for the error path (NG1) — logged only.
+- No changes to pull-time detection, the detail-panel warning, or recurrence math.
+- Not fixing the underlying throwers.
+
+## Decisions
+
+### D1 — Add a fourth union member `error_creating_copy` (vs. reusing `skipped_invalid_rule` or a boolean flag)
+
+Extend `RecurringResult` with `{ status: 'error_creating_copy' }`. The early `if (!rule)` return keeps `skipped_invalid_rule`; the `catch` block returns `error_creating_copy` (still `console.error`-logging, still returning `finalCompletedTask`).
+
+- **Why a new member:** it makes the two causes type-distinct, so every `switch`/`if` over `recurringResult.status` is forced (by the discriminated union) to consider the new case — surfacing exactly the four caller sites that need review. Reusing `skipped_invalid_rule` can't fix the mislabel; a side boolean (`wasError`) would bypass exhaustiveness and re-introduce drift.
+- **Naming:** `error_creating_copy` reads as "the copy couldn't be created due to an error", parallel to the existing snake_case statuses.
+
+### D2 — Callers ignore `error_creating_copy` for alerting (no new alert)
+
+The live hooks currently do `if (recurringResult.status === 'skipped_invalid_rule') addAlerts([{ type: 'repeat_rule_invalid', ... }])`. That condition is already exact-match on `skipped_invalid_rule`, so **it needs no change to stop firing** for `error_creating_copy` — but each site gets a characterization test pinning "error status → no alert", and a short comment, so the behavior is intentional and guarded rather than incidental.
+
+- **Alternative considered:** add a truthful generic alert for the error path. Deferred (NG1/Q1) to keep the change surgical; silent-but-logged matches the pre-existing "don't interrupt completion" intent, only without the *wrong* message.
+
+### D3 — Extract a shared `useTaskCompletionAlerts` helper (FR5, FR6)
+
+The impact scan (task 1.1) found the `if (recurringResult.status === 'skipped_invalid_rule') addAlerts(...)` block copy-pasted, with minor variations, across four completion hooks (`useTasks`, `useInboxTasks`, `useTaskMutations`, plus the dead `useTask` — removed per D5) — and found a further call site, `SearchPage.tsx`, that discards `recurringResult` entirely and never alerts at all. This is the concrete "same action, different behavior per page" symptom the user flagged.
+
+Extract a single helper, `packages/client/src/hooks/useTaskCompletionAlerts.ts` (100–200 lines, alongside the existing `useMutationHelpers.ts`), that takes `recurringResult` + the task name and calls `addAlerts` exactly once, with the `error_creating_copy` case producing no alert (per D2) and `skipped_invalid_rule` producing the existing `repeat_rule_invalid` alert. The three live hooks replace their inline block with a call to this helper; `SearchPage.tsx`'s `handleCompleteTask` is updated to call it too, closing the silent-failure gap.
+
+- **Why extract now, not defer:** the duplication is what let the sites drift and let `SearchPage.tsx` skip alerting entirely undetected. Fixing FR1–FR4 without touching the duplication would just add one more place (`error_creating_copy` handling) to keep in sync by hand.
+- **Why not a bigger refactor:** `useGoalTasks.ts` already composes `useTaskMutations(...)` rather than reimplementing it — this follows the same existing composition pattern instead of introducing a new architecture (e.g. no `hooks/index.ts` barrel, no change to how `completeTask` is threaded through components). See NG5.
+- **Naming:** `useTaskCompletionAlerts` — it's a hook only insofar as it may call `useAlerts()` internally; call sites use it as `raiseCompletionAlerts(recurringResult, taskName)` or similar plain function returned from the hook.
+
+### D4 — Spec deltas mark the intent change
+
+`invalid-repeat-rule-detection` and `repeating-tasks` deltas convert the "exception → skipped_invalid_rule" scenarios to "exception → error_creating_copy" and add "error path shows no invalid-rule alert". This is the source-of-truth change; the code follows it.
+
+### D5 — Delete the dead `useTask` hook instead of refactoring it (FR7)
+
+A follow-up usage scan (recorded in task 2.1) found `useTask.ts` has no importers anywhere in `packages/client/src` — only `useTask.test.ts` references it. No page or component consumes the hook; its functionality is covered by `useTaskMutations`/`useTasks`.
+
+Delete `useTask.ts` + `useTask.test.ts` early (section 2 of tasks, before the union extension), so FR3/FR5 and the mutation-testing scope cover only live code.
+
+- **Alternative — refactor it along with the live hooks:** rejected; spending TDD + mutation-testing effort (>=95% score) on code no user path can reach is waste.
+- **Alternative — a separate removal change:** rejected; the file is one of the copy-pasted alert sites, squarely inside this change's blast radius. Deleting it first shrinks this change instead of coordinating two changes over the same lines.
+
+## Risks / Trade-offs
+
+- [A consumer does a non-exhaustive `switch` on `recurringResult.status` and silently drops the new case] → TypeScript's discriminated union + the existing `not_recurring`/`created` handling make the live call sites explicit; a repo grep for `recurringResult.status` / `skipped_invalid_rule` in the tasks confirms every consumer is reviewed.
+- [`useTask` is secretly used (dynamic import, planned feature) and its deletion breaks something] → the grep in task 2.1 (word-boundary, whole `src`) found zero importers; `pnpm run build` + the unit suite after deletion are the guard. If a future feature needs a single-task hook, it can be recreated on top of `useTaskCompletionAlerts`.
+- [Silent failure: the next occurrence isn't created and the user sees nothing] → Same visibility as today for the error path (it was already swallowed); the only change is removing a *false* explanation. Q1 tracks a future truthful alert.
+- [Type change ripples to the in-memory adapter or contract types] → `RecurringResult` is a client-local type in `TaskService.ts`; verify no re-export in `packages/contract`. If it is shared, extend there too (task 1.x covers the grep).
+- [`SearchPage.tsx` behavior changes user-visibly for the first time — a genuinely invalid rule now alerts there when it silently didn't before] → intentional per FR6/U3; guarded by a characterization test showing prior silent behavior, then a test showing the new alert.
+
+## Migration Plan
+
+Pure additive type change + branch split, plus an extraction of existing duplicated logic into one helper (no behavior change to the three live hooks; `SearchPage.tsx` gains alerting it previously lacked) and deletion of the dead `useTask` hook (no runtime impact — nothing imports it). No data migration, no persisted-shape change, no server change. Rollback = revert the commit. Deploys with the normal client build.
+
+## Open Questions
+
+- Q1 (from proposal): whether `error_creating_copy` should later surface a truthful "couldn't create the next occurrence" alert. Out of scope here.

openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/proposal.md

@@ -0,0 +1,108 @@
+# fix-recurring-completion-error-masking
+
+## Why
+
+`TaskService.complete()` wraps recurring-copy creation in a broad `try/catch` that returns `{ status: 'skipped_invalid_rule' }` for **any** thrown error — even when the `repeat_rule` parsed fine and the failure was something else entirely (a DB write error, an invalid `sort_order` key, etc.). Completion hooks then show the user a "Rule not recognized / could not be recognized" alert, blaming the repeat rule for an unrelated failure. During the `fix-stale-sync-overwrites` work this misleading dialog actively masked the real cause of a failure (an invalid seeded sort key throwing in `generateKeyBetween`), costing debugging time and confusing the user with a wrong diagnosis.
+
+## What Changes
+
+- **MODIFIED**: `TaskService.complete()` returns a new, distinct `recurringResult` status for an unexpected exception during copy creation, separate from the genuine "rule failed to parse" case. `skipped_invalid_rule` is returned **only** when `parseRepeatRule()` returns `null`.
+- **MODIFIED**: Completion callers (`useTasks`, `useInboxTasks`, `useTaskMutations`) no longer raise the `repeat_rule_invalid` alert for the exception path — that alert stays reserved for genuinely invalid rules. The exception is still caught (completion never fails) and logged.
+- **ADDED**: A shared completion-alert helper (`useTaskCompletionAlerts` or equivalent) replaces the three live copy-pasted `if (recurringResult.status === 'skipped_invalid_rule') addAlerts(...)` blocks in `useTasks`, `useInboxTasks`, `useTaskMutations`. Scope expansion decided after the impact scan (task 1.1) found this duplication — the same completion action was behaving inconsistently across call sites because the alerting logic was copy-pasted rather than centralized.
+- **ADDED**: `SearchPage.tsx`'s `handleCompleteTask` — previously discarding the `complete()` return value entirely with no alert handling — now routes through the same shared helper, closing a silent-failure gap discovered during the impact scan.
+- **REMOVED**: The dead `useTask` hook (`packages/client/src/hooks/useTask.ts` + `useTask.test.ts`) is deleted. A follow-up usage scan found it is imported nowhere in the app — only its own test references it — so it carried a fourth copy of the alert block without affecting any user-facing behavior. Deleting it (instead of refactoring dead code) shrinks the completion call-site list to three hooks plus `SearchPage.tsx`.
+- **UNCHANGED (guarded by tests)**: Completion always succeeds regardless of `recurringResult`; the pull-time invalid-rule detection (`SyncProvider` → `filterTaskNamesWithInvalidRepeatRules`) and the task-detail-panel "Rule not recognized" warning are untouched; recurrence math is untouched.
+
+## Capabilities
+
+### New Capabilities
+
+(none)
+
+### Modified Capabilities
+
+- `invalid-repeat-rule-detection`: "Completion returns discriminated union for recurring result" — the exception-during-copy-creation path returns a distinct status (not `skipped_invalid_rule`); "Alert shown on completion when recurring copy skipped due to invalid rule" — the `repeat_rule_invalid` alert fires only for genuinely invalid rules, not for the exception path.
+- `repeating-tasks`: "System creates a recurring copy on task completion" — the `recurringResult` union gains a status for the copy-creation-error path; `skipped_invalid_rule` no longer covers "an exception occurred during copy creation".
+
+## Goals
+
+- G1: A completion where the repeat rule is valid but copy creation throws never tells the user their repeat rule is invalid.
+- G2: `skipped_invalid_rule` (and its "Rule not recognized" alert) is a truthful signal — it appears if and only if `parseRepeatRule()` returned `null`.
+- G3: Completion behavior is otherwise byte-for-byte unchanged: the task is still marked complete, the error is still swallowed (never interrupts completion) and logged.
+- G4: Every completion call site reacts to `recurringResult` the same way, through one shared helper — no call site independently reimplements or omits the alert logic.
+
+## Non-Goals
+
+- NG1: No new user-facing alert/dialog for the copy-creation-failure path. The unexpected error is logged (`console.error`) and completion proceeds silently; surfacing a dedicated "couldn't create the next occurrence" alert is a possible follow-up, out of scope here.
+- NG2: No change to pull-time invalid-rule detection or the task-detail-panel warning label.
+- NG3: No change to recurrence math, skip logic, or next-date/appear-date calculation.
+- NG4: Not fixing the underlying throwers (e.g. invalid `sort_order` keys) — this change only stops mislabeling them; the sort-key seeding fix already landed in `fix-stale-sync-overwrites`.
+- NG5: No broader refactor of hook architecture, no `hooks/index.ts` barrel introduction, no change to how `completeTask`/`onComplete` is passed through components (`TaskItem`, `TaskList`, detail pages) — those remain plain passthrough. Only the alert-reaction logic is extracted.
+
+## Users & Scenarios
+
+- U1: A user completes a recurring task whose repeat rule is perfectly valid, but an internal error prevents the next occurrence from being created. The task is marked complete and the user is NOT shown a false "Rule not recognized" message.
+- U2: A user completes a task whose repeat rule really is corrupt (unparseable). They still see the truthful "Rule not recognized" alert — unchanged from today.
+- U3: A user completes a task with a genuinely invalid repeat rule from the search results page. They now see the same truthful "Rule not recognized" alert they'd see from any other page, instead of silence.
+
+## Requirements
+
+### Functional
+
+- FR1: `TaskService.complete()` SHALL return `{ status: 'skipped_invalid_rule' }` only when `parseRepeatRule(existingTask.repeat_rule)` returns `null`.
+- FR2: When an exception is thrown after the rule parsed successfully (during next-date calculation or copy creation/update), `TaskService.complete()` SHALL catch it, log it via `console.error`, and return a distinct status `{ status: 'error_creating_copy' }` (name finalized in design). The completed task SHALL still be returned and marked complete.
+- FR3: Completion callers that currently map `skipped_invalid_rule` → `repeat_rule_invalid` alert (`useTasks`, `useInboxTasks`, `useTaskMutations`) SHALL NOT raise that alert for `error_creating_copy`.
+- FR4: The `RecurringResult` discriminated union SHALL be extended (not repurposed) so exhaustive `switch`/`if` handling stays type-safe; existing `created` / `not_recurring` handling is unchanged.
+- FR5: The `skipped_invalid_rule` → `repeat_rule_invalid` alert logic SHALL be extracted into one shared helper, used by all completion call sites (`useTasks`, `useInboxTasks`, `useTaskMutations`), replacing the three independent copy-pasted implementations.
+- FR6: `SearchPage.tsx`'s task-completion handler SHALL use the same shared helper, so a genuinely invalid rule raises the `repeat_rule_invalid` alert there too, consistent with every other completion entry point.
+- FR7: The dead `useTask` hook (`useTask.ts`, `useTask.test.ts`) SHALL be deleted, after re-verifying via grep (recorded in tasks) that nothing outside its own test imports it; `pnpm run build` and the unit suite SHALL stay green after deletion.
+
+### Non-Functional
+
+#### Performance
+
+- Not applicable — no new queries or loops.
+
+#### Accessibility
+
+- Not applicable — no new UI; a misleading alert is suppressed, not added.
+
+#### Responsive
+
+- Not applicable — no UI layout change.
+
+## UX Acceptance Criteria
+
+- UX1: After completing a recurring task whose rule is valid, the user never sees a "Rule not recognized" / "Repeat rule issue" alert caused by an unrelated internal error.
+- UX2: After completing a task whose rule is genuinely unparseable, the "Rule not recognized" alert still appears exactly as before.
+- UX3: Completing a task with a genuinely invalid rule from the search page now shows the same "Rule not recognized" alert as everywhere else, instead of no feedback.
+
+## UI States Matrix
+
+No UI states change. The only user-visible effect is the ABSENCE of a wrong alert on the exception path; existing loading/error/empty/offline states are unaffected.
+
+## Behavior
+
+Scenarios live in feature files tagged `@fix-recurring-completion-error-masking @FR-X`:
+
+- `packages/client/src/test/features/repeating_tasks/complete_copy_error.feature` — FR1, FR2, FR3 (unit BDD, vitest-cucumber): valid-rule-but-copy-throws → `error_creating_copy`, no invalid-rule alert; unparseable-rule → `skipped_invalid_rule` + alert.
+
+## Visual Reference
+
+Not applicable — no UI changes.
+
+## Affected IA
+
+No changes.
+
+## Success Metrics
+
+- M1: Unit test proves an exception thrown during copy creation with a valid rule yields `error_creating_copy` and raises no `repeat_rule_invalid` alert.
+- M2: Existing `skipped_invalid_rule` scenarios (parse returns null) stay green unchanged — the truthful alert path is preserved.
+- M3: Mutation score on the changed `complete()` catch/branch and the caller alert mapping >= 95% (minimum acceptable 90%).
+- M4: All four completion call sites (`useTasks`, `useInboxTasks`, `useTaskMutations`, `SearchPage.tsx`) call the same shared helper — zero duplicated `if (recurringResult.status === 'skipped_invalid_rule')` blocks remain (verified by grep in task 7.6).
+- M5: `useTask.ts` and `useTask.test.ts` are absent from the codebase; a grep for `useTask` (word-boundary, excluding `useTasks`/`useTaskMutations`/`useTaskCompletionAlerts`) over `packages/client/src` returns zero matches (FR7).
+
+## Open Questions
+
+- Q1: Should `error_creating_copy` eventually surface a truthful, generic "couldn't create the next occurrence" alert instead of failing silently? Deferred to NG1; revisit if users report silent failures.