fix(snapshot): report a refused cron merge to the import caller

[chenmingwei23]

Problem / Motivation

A snapshot import whose cron merge was refused is reported to the caller as a successful one. _merge_crons (src/kiro_crew/snapshot.py) has three refusal paths -- unreadable source JSON, unreadable destination JSON, and an unusable cron shape on either side -- and each returned None with only a print to show for it. apply_import_zip (src/kiro_crew/portability.py) then appended "crons (merged)" to the summary unconditionally, and the dashboard handler logged the import with outcome="ok". Net effect: an import that restored ZERO cron jobs answered {ok: true} with a summary listing "crons (merged)", and the SEL audit trail agreed. The only trace of the refusal was a print a dashboard import has no terminal to show.

Why it matters

A user restoring a backup expecting their scheduled jobs back is told it worked when no job was imported. They discover the loss later, when a job silently never fires -- and the audit trail, the one place that should contradict the false success, confirms it instead.

What changed (motivation -> approach -> change)

Symptom: false "crons (merged)" over a refused merge. Root cause: the merge's outcome existed only on stdout, so no caller could distinguish a merge from a refusal. Fix: make the refusal observable at each caller, minimally:

• _merge_crons now returns bool: True only after the merged store is written, False on all three refusal paths. Diagnostics stay on stdout; a failing write still raises (OSError propagates to the handler's error path, which is the correct loud behavior).
• apply_import_zip gates the summary item on that return: a refusal appends "crons (skipped: unreadable or invalid cron store)" plus a machine-readable refused_merges: ["crons"], instead of "crons (merged)".
• The dashboard handler (src/kiro_crew/dashboard/handlers/portability.py) logs outcome="partial" naming the refused component in resources, instead of a flat ok. "partial" follows the established convention (connections.py, security.py, hooks_integration.py).
• The CLI restore path (_do_merge) prints an explicit crons: merge skipped line instead of "OK checkmark" crons status line over a refusal.

Known, deliberately out-of-scope siblings (this PR stops the cron-merge false positive at its root, per the issue; each of these is the same shape but a different surface):

• PortabilityTab.tsx renders only summary.items.length with a green check, so the human-visible toast is unchanged; the API payload and audit trail are now truthful, and refused_merges is in the response for the UI to render. Precedent: rejected_crons / paused_crons are equally unrendered today. Frontend surfacing is a small follow-up (this change is backend-only by design).
• An archive whose own cron store is unreadable is rewritten to an empty store by _sanitize_imported_crons before the merge, so that case still reports "crons (merged)" over zero restored jobs -- the truth for that path lives in rejected_crons, which the sanitizer already reports. Pre-existing, not introduced or widened here.
• _merge_memory and _merge_notifications append "(merged)" under the same unconditional pattern; left alone deliberately to keep this root-cause scoped to the component the issue names.
• The CLI restore's machine-readable terminal event (state_restored) does not yet reflect a refused component; _do_merge returns None today and propagating it is a second contract change.

Tests

• test_merge_crons_returns_the_outcome_on_every_path: all three refusal paths answer False and write nothing; a genuine merge answers True and writes the merged store.
• test_a_refused_cron_merge_is_not_reported_as_merged (parametrized x3): every live-store refusal reachable through apply_import_zip (unreadable bytes, non-object top level, unusable job list) yields the skip item + refused_merges, never "crons (merged)", and leaves the live store byte-identical.
• test_an_archive_side_refusal_is_not_reported_as_merged: the one archive-side refusal that survives the sanitizer end-to-end (a lone-surrogate job name) is reported as a skip.
• test_a_genuine_cron_merge_still_reports_merged: no regression on the success path.
• test_import_handler_outcome_reflects_a_refused_merge (parametrized x2): the handler logs partial + refused=crons for a refused merge and ok for a clean one.
• TestARefusedCronMergeIsVisibleInTheRestoreStatus (2 tests): _do_merge prints the skip line, not "OK checkmark" crons status line, over a refusal -- and still prints the checkmark on a genuine merge.

Local gates: isort / flake8 / mypy clean; test/test_portability.py + test/test_snapshot.py 132 passed; full backend suite run -- the only failures are pre-existing host-environment classes (NFS /local/home ownership, AF_UNIX path too long) that reproduce identically on a pristine origin/main worktree on this host.

Manual verification

N/A -- unit coverage sufficient: both production callers of _merge_crons and the handler outcome are exercised directly by the new tests, and the change has no UI surface.

Closes #8217

Pattern harvest

Rule candidate: a helper that can refuse its work (merge/restore/import) must return the outcome, and a caller MUST NOT append a success item to a user-facing summary (or log outcome=ok) unless that return says the work happened. Two sibling instances of the same shape exist today (_merge_memory, _merge_notifications append "(merged)" unconditionally) and are named in the out-of-scope list above -- candidate for an AUTOSDE reviewer rule on summary["items"].append(...(merged)...) sites adjacent to an ungated helper call; a semgrep rule is likely too syntactic to avoid false positives.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 5b3182d15adef06c9e97d9a4be7c41fe6e74608b — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix at the right layer: the helper returns its outcome, both callers gate reporting on it, and the audit trail follows an existing partial convention.

Suggestions

• The named siblings (_merge_memory, _merge_notifications, and the sanitizer's "merged over zero jobs" path) leave the same false-success shape live on adjacent surfaces — file the follow-up now so the pattern-harvest rule has a tracked home rather than a PR-description grave.

[DESIGN-REVIEWED] 5b3182d

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5b3182d15adef06c9e97d9a4be7c41fe6e74608b — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 5b3182d

Verdict parsed from the review's SHA-scoped output markers for commit 5b3182d15adef06c9e97d9a4be7c41fe6e74608b.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5b3182d15adef06c9e97d9a4be7c41fe6e74608b: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 5b3182d15adef06c9e97d9a4be7c41fe6e74608b — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. Composing the review.

First-Principles-Verdict: CONCERNS

The fix earns its place at both callers, but refused_merges ships as a list with one consumer and one value ever appended, and the declared sibling count is off by one.

What this change ships

Intent: stop a snapshot import from reporting a refused cron merge as a success — a FIX.

1. Import summary says "crons (skipped: …)" instead of "crons (merged)" over a refusal — justified (defect snapshot: a refused cron merge is reported to the import caller as a successful one #8217).
2. API response gains a refused_merges field — one consumer, generalized.
3. Audit log records the import as partial, naming the refused component — justified (convention counted: 3 existing outcome="partial" sites).
4. CLI restore prints a skip warning instead of "✅ crons" over a refusal — justified (second of the 2 production callers, same defect).
5. _merge_crons returns its outcome — the fix's mechanism, justified.
6. cron.py cross-reference doc updated — rides along; mandated same-commit doc sync.

Watch

• The description's out-of-scope list claims two siblings "append '(merged)' under the same unconditional pattern"; only one is real. _merge_notifications raises on refusal (its docstring: "ABORT, never skip… this function's callers report an outcome"), so its append is reached only on success. The true unfixed sibling is _merge_memory alone — two silent print-and-return refusals (snapshot.py:2255, snapshot.py:2258) behind the unconditional append at portability.py:541. Declared and deferred; count grepped, not disputed.

Subtractions

• Shrink refused_merges: ["crons"] to a singular flag: grep refused_merges shows one real consumer (dashboard/handlers/portability.py:137) and one value ever appended (portability.py:555). The list form exists only for the deferred memory fix; let that PR generalize it.

[FIRST-PRINCIPLES-REVIEWED] 5b3182d

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 5b3182d15adef06c9e97d9a4be7c41fe6e74608b and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5b3182d

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 5b3182d15adef06c9e97d9a4be7c41fe6e74608b: <one-sentence reason>

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #6066 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6066: REBASE. Complementary changes to one handler with disjoint hunks; whichever lands second needs no rework beyond ordinary context adjacency. Files: src/kiro_crew/dashboard/handlers/portability.py.
• PR #8184 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8184: KEEP. Complementary halves of one reported defect class in the same file, by the same author, with disjoint functions. Neither subsumes the other; only the second to land needs a positional rebase of test/test_snapshot.py. Files: src/kiro_crew/snapshot.py, test/test_snapshot.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Correct root-cause fix for a genuine false-success: _merge_crons returns bool (False on all three refusal paths, True only after the merged store is written), and both production callers now gate on it — apply_import_zip emits a skip item plus machine-readable refused_merges instead of an unconditional "crons (merged)", and the dashboard handler logs outcome="partial" naming the component instead of a flat ok. The distinction I checked most closely is the one the docstring makes explicit and the code honours: a failed WRITE still raises OSError rather than being flattened into False, so a loud failure stays loud. _do_merge's no-crons-in-archive path also still prints the checkmark, so the skip line only appears over a real refusal. Tests cover all three refusal paths, both callers, the handler outcome, and the success path.