fix: resolve critical and major issues from code review

[vscarpenter]

Summary

Fixes 1 critical and 5 major issues identified during comprehensive code review of commit fd2ffc8.

• [Critical] Add dryRun to bulk_update_tasks — was silently mutating data when dryRun: true was passed; the schema accepted it but the implementation dropped it
• [Major] Separate deleted vs updated counts — bulk operations now correctly report "Updated: N" and "Deleted: N" instead of counting all as "Updated"
• [Major] Fix fallback totalTasks semantics — fallback path returned pendingPushCount + pendingPullCount as total tasks (wrong); now returns null for unknown counts
• [Major] Thread request Origin through auth middleware — all 5 errorResponse calls in auth.ts now pass the request origin, fixing CORS on auth errors in dev
• [Major] Add environment param to CORS helpers — jsonResponse, errorResponse, createCorsHeaders now accept optional environment to activate dev-port allowlist
• [Major] Optimize filtered query caching — filtered listTasks calls now check the cached all-tasks list first, avoiding redundant network + decrypt round trips

Additional improvements

• initializeEncryption() called once per batch instead of per-task (perf fix)
• Fixed import ordering violation in list-tasks.ts
• maxTasks uses ?? instead of || to correctly handle 0

Test plan

• bun typecheck — clean
• bun lint — clean
• bun run test — 1,713 tests passing (89 suites)
• Manual: verify bulk_update_tasks with dryRun: true returns preview without mutating
• Manual: verify auth errors in dev return correct CORS origin header

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

PR Review — fix: resolve critical and major issues from code review

Overall this is a solid set of targeted fixes. The critical dryRun bug, the double-counted bulk results, and the meaningless fallback totalTasks are all genuine correctness problems worth addressing. A few observations below, ranging from a real gap to minor nits.

---

What's well done

• dryRun threading in bulk_update_tasks — the fix is complete and correct. The schema already accepted the flag but it was silently dropped; now it's properly forwarded all the way to bulkUpdateTasks(), and the handler produces distinct "Would update / Would delete" output.
• Separate update vs. delete counts — counting operations.filter(op => op.type === 'delete').length and deriving updateCount = operations.length - deleteCount is clean and accurate.
• totalTasks: null on fallback — returning typed nulls instead of the semantically-wrong pendingPushCount + pendingPullCount is the right call. The schema change to .nullable() flows through correctly.
• initializeEncryption once per batch — moving it out of the per-task loop in decryptTaskBatch is a good perf fix, and decryptSingleTask can be safely deleted now that it's inlined.
• Math.max(...spread) → reduce — correct fix for stack-overflow risk on large arrays.
• maxTasks ?? 50 instead of || — handles the zero case correctly.
• PWA controllerchange cleanup — clearing the fallback timeout and removing the event listener before reloading prevents a double-reload on iOS. Nice catch.
• Double encodeURIComponent removal in response-builder.ts — URLSearchParams.set() already percent-encodes the value; wrapping it in encodeURIComponent first caused double-encoding in the redirect URL.
• Device type coercions — explicit String() and Number() casts on D1 query results are defensive and correct.
• Structured log object for all levels — consistent logObject across debug/info/warn/error is a good improvement.

---

Issue: environment param is never passed in the auth middleware fix

The PR summary says "Thread request Origin through auth middleware — fixing CORS on auth errors in dev." The origin is now correctly threaded — that part works. But the environment param added to errorResponse / getCorsHeaders is never forwarded from auth.ts, so the dev-port allowlist (localhost:3000, 5173, etc.) still won't fire for auth errors.

Current auth.ts:

return errorResponse('Missing or invalid Authorization header', 401, origin);
// environment not passed → isOriginAllowed(origin, undefined) → dev ports excluded

What it should be (if the intent is to allow dev origins through auth errors):

return errorResponse('Missing or invalid Authorization header', 401, origin, env.ENVIRONMENT);

env is already in scope as a parameter to authMiddleware. Since the PR explicitly calls this out as a dev CORS fix, this gap seems unintentional. If the intent is only to reflect the request's own origin (i.e. not widen the allowlist), the new environment param on these helpers is dead code for this use case and should be documented.

---

Cache optimization misses the write path for filtered requests

In list-tasks.ts, the optimized cache logic is:

if (filters) {
  const cachedAll = cache.getTaskList('all');
  if (cachedAll) {
    return applyTaskFilters(cachedAll, filters);  // cache hit
  }
}
// ... fetch + decrypt ...
if (!filters) {
  cache.setTaskList(cacheKey, decryptedTasks);    // only unfiltered writes to cache
}

If a filtered request arrives before any unfiltered request has populated the 'all' cache, it fetches from the API but never writes to the 'all' cache. The next filtered request hits the API again. The optimization only works when an unfiltered call has already warmed the cache.

Consider also caching the full result set on filtered fetches:

// After decrypt, cache the full list regardless of filters
cache.setTaskList('all', decryptedTasks);
return filters ? applyTaskFilters(decryptedTasks, filters) : decryptedTasks;

This makes the cache warm for any subsequent request regardless of call order.

---

applyTaskFilters will throw if task.tags is undefined

!filters.tags.some((tag) => task.tags.includes(tag))
//                          ^^^^^^^^^^
// throws if task.tags is undefined (possible for older tasks pre-tags feature)

The schema marks tags as optional with a default of [], but decrypted blobs from old synced data may not have the field populated. A simple guard avoids the crash:

!filters.tags.some((tag) => (task.tags ?? []).includes(tag))

---

Minor observations

Debug level condition in MCP logger

const isDebugEnabled = process.env.LOG_LEVEL === 'debug' || process.env.NODE_ENV === 'development';

The || process.env.NODE_ENV === 'development' half means debug logs always fire in dev even if no one opted in. For an MCP stdio server this adds noise to every session. Consider requiring LOG_LEVEL=debug explicitly instead of the OR condition.

sync-status.ts catch block swallows schema validation errors

If Zod .parse() throws (e.g. server returned a different shape), it falls through to the warn log and the null-returning fallback path. This could mask a real API contract mismatch. Worth logging the error type so schema failures are distinguishable from network failures.

config.ts log level downgrade

Changing the guidance text from logger.error to logger.info is correct — it's help text, not an error event. logger.warn might be a slightly better fit since throw error immediately follows and something did go wrong.

---

Summary

Area	Status
dryRun bug fix	Correct
Bulk result counts	Correct
totalTasks fallback semantics	Correct
Origin threading in auth middleware	Correct
environment param wiring in auth	Incomplete
Cache write path for filtered queries	Misses warm on first filtered call
task.tags undefined guard	Can throw on legacy data
Math.max spread fix	Correct
PWA listener cleanup	Correct
Double encodeURIComponent fix	Correct
Device type coercions	Correct
Structured logging consistency	Correct

The three items marked Incomplete/Can throw/Misses are worth addressing before merge — especially the task.tags guard (runtime crash risk on legacy data) and the cache write-path gap (the optimization doesn't deliver its benefit when a filtered call runs first). The environment threading gap is lower priority if dev CORS was working through other means, but worth clarifying either way.

🤖 Generated with Claude Code