@qwen-code /triage

@qwen-code /triage

Thanks @tt-a1i for the fix!

Template looks good now ✓ — all required sections present.

On direction: this solves a real data-loss bug. readCronTasks() silently dropping invalid entries, then updateCronTasks() persisting the filtered list, permanently loses user data. Aligning invalid-entry handling with the existing malformed-JSON and non-array behavior (fail closed, refuse to write through corruption) is clearly the right call. category/core and scope/session-management territory.

On approach: the scope is tight — one function changed from filter + warn to iterate + throw, unused import cleaned up, two targeted tests. No scope creep, no drive-by refactors. Exactly the minimal change the bug demands.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal: Same approach — replace the silent filter(isValidTask) + debugLogger.warn() with a loop that throws on the first invalid entry, aligning with the existing fail-closed behavior for malformed JSON and non-array files. Remove the now-unused createDebugLogger import.

Comparison with the PR: The implementation matches exactly. The for...of parsed.entries() loop is clean, the error message includes the index and file path for debuggability, and the debugLogger removal is correct (it was the only call site in the file). No simpler path was missed.

Findings: No correctness bugs, security issues, or AGENTS.md violations. The diff is focused — every line changed serves the stated goal. The type narrowing works because every entry must pass the isValidTask type guard or the function throws before returning.

Test Results

22 tests pass (up from 21 on main — one new test added, one modified):

 RUN  v3.2.4 packages/core
      Coverage enabled with v8

 ✓ src/services/cronTasksFile.test.ts (22 tests) 201ms

 Test Files  1 passed (1)
      Tests  22 passed (22)
   Start at  20:02:58
   Duration  4.25s

New test — refuses to clobber a file with invalid task entries: seeds a file with [validTask, {id: 'bad'}], calls updateCronTasks with a mutation that adds a task, asserts the read throws /Invalid task entry/ and the on-disk bytes are unchanged. This directly validates the data-loss prevention the PR claims to fix.

Modified test — throws for invalid task entries (was filters out invalid entries): same seed, but now asserts readCronTasks rejects instead of returning a truncated array.

Evidence (Before & After): N/A — non-visible storage-validation logic, covered by unit tests above.

 RUN  v3.2.4 packages/core
      Coverage enabled with v8

 ✓ src/services/cronTasksFile.test.ts (22 tests) 201ms

 Test Files  1 passed (1)
      Tests  22 passed (22)
   Start at  20:02:58
   Duration  4.25s

— Qwen Code · qwen3.7-max

Stepping back: this is a clean, well-scoped fix for a real data-loss bug. The motivation is solid — silently dropping entries and then persisting the truncated list is the kind of corruption that's painful to debug when it bites. The fix aligns perfectly with the existing fail-closed philosophy already used for malformed JSON and non-array files.

The implementation matches what I'd write independently. The diff is +23/-22 — almost a wash — and every line earns its place. The for...of parsed.entries() loop is more readable than the old filter + warn combo, the error message tells the user exactly which entry is bad and where to find it, and the debugLogger cleanup is the right housekeeping.

The new refuses to clobber test is the standout — it doesn't just verify that the read throws, it verifies that the on-disk file is byte-for-byte unchanged after a failed update. That's the actual property users care about: "my file won't get clobbered."

All 22 tests pass. No correctness concerns, no convention violations, no scope creep. This is the kind of PR that's easy to maintain — focused, well-tested, and does exactly what it says.

Approving. ✅

— Qwen Code · qwen3.7-max

✅ Local build & verification — LGTM, ready to merge

Built the real artifacts from this PR's HEAD and verified the fail-closed behavior end-to-end on a real filesystem, including how the existing callers handle the new throw. Correct, well-scoped, and well-guarded by tests. Recommending merge.

What this PR does

readCronTasks() now throws on the first invalid task entry in an otherwise-valid array (Invalid task entry at index <n> in <file> …) instead of silently filter()-ing it out. This stops a later updateCronTasks() from persisting the filtered list and permanently deleting a recoverable, hand-edited entry — aligning invalid-entry handling with the existing malformed-JSON / non-array behavior (fail closed, refuse to write through corruption). The now-unused debug logger is removed. Fixes #5308.

Environment

• macOS (Darwin 25.5.0), Node v22.22.2, npm 10.9.7
• Worktree on PR HEAD da603b3d08; base origin/main (31 commits behind but MERGEABLE; the 2 changed files are clean against origin/main).

Real-filesystem behavior (actual source, isolated via Storage.setRuntimeBaseDir, no ~/.qwen touch)

Verification matrix

Callers handle the new throw gracefully (the key safety check)

readCronTasks already threw for malformed-JSON / non-array; this PR extends the same contract to invalid entries. Both production callers were already designed for it:

• cron-list.ts wraps the read in try/catch and surfaces a clean error result to the user (Error listing cron jobs: …) — no crash.
• cronScheduler.ts loadFileTasks() catches and keeps its current in-memory view, retrying on a later reload (explicitly avoids wiping loaded durable jobs). updateCronTasks() reads before writing, so a throw means the write never happens — the file is preserved for recovery.

Behavioral tradeoff (deliberate, non-blocking)

With this change, a single invalid entry now blocks the whole schedule from loading (and surfaces an error in cron list) until the user fixes/deletes it, rather than silently loading the valid subset. That is the intended fail-closed tradeoff — identical to how malformed-JSON / non-array files already behave — and is preferable to silently and permanently losing a recoverable entry on the next write. The error names the exact index to fix.

Verdict: LGTM ✅ — a correct, consistent data-safety fix; callers degrade gracefully and the two new tests genuinely guard the behavior.