Skip to content

Report effective completion date on parent-completed subtasks - #6

Open
vishae wants to merge 6 commits into
steveardis:mainfrom
vishae:bug-003-effective-completion-date
Open

Report effective completion date on parent-completed subtasks#6
vishae wants to merge 6 commits into
steveardis:mainfrom
vishae:bug-003-effective-completion-date

Conversation

@vishae

@vishae vishae commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Stacked on #2 and #5 — this branch is built on top of tech-017-drop-defer-sequential (open as #5, itself stacked on #2), so this diff currently includes both of those commits too. Once #2 and #5 merge, this PR's diff will shrink to just the commit below.

Completing a parent task effectively completes its children too, but a child's own completionDate stays null — only the parent's own completionDate gets set. Any completedAfter scan (list_tasks {status: ["complete"], completedAfter}) therefore silently misses parent-completed subtasks, since a null completionDate can never satisfy the filter.

Switched every task snippet's completionDate output (list_tasks/get_task/create_task/edit_task/drop_task/complete_task/move_task) from task.completionDate to task.effectiveCompletedDate, which OmniFocus populates from the parent's completion when the task's own is unset.

Verified before shipping that this introduces no regression:

  • For a normally-completed (non-child) task, effectiveCompletedDate matches completionDate exactly.
  • For a repeating task, tested completing an instance directly against a live OmniFocus database: the completed instance and the newly-spawned next occurrence both show completionDate/effectiveCompletedDate in agreement (no divergence), so this fix doesn't interact badly with recurrence.

Test plan

  • npm test — 151 unit tests pass
  • npm run test:integration — 99/99 pass, including new dedicated coverage (effectiveCompletionDate.int.test.ts) reproducing the exact parent/child scenario
  • Rebuilt, packed, and installed the .mcpb locally; verified live against a real OmniFocus database — completing a parent task now makes get_task(child) return a real completionDate, and list_tasks {status:["complete"], completedAfter} correctly returns both parent and child (previously only the parent)

vishae added 5 commits July 10, 2026 04:22
…erId null on completed tasks

list_tasks didn't expose a completed task's completionDate (only get_task
did), and there was no way to filter by completion date server-side.
Added completionDate to list_tasks' output and a completedAfter (ISO
datetime) filter, following the existing dueBeforeDate pattern.

Separately, list_tasks and get_task both resolved a task's containerId/
containerType via assignedContainer, which comes back null for completed
tasks. Other write-path snippets (move_task, complete_task, create_task,
edit_task, drop_task) already avoid this via parentTask/containingProject
instead -- move_task.js even has a comment noting assignedContainer
"doesn't update after moveTasks". Applied the same fix to list_tasks and
get_task so completed tasks resolve their container correctly too.

package-lock.json also resyncs to package.json's current name/version
(0.1.0 -> @scardis/omnifocus-mcp@0.1.2), which had drifted.
…tial field

Closes TECH-017 Gaps 1 & 2:
- list_tasks/get_task/create_task/edit_task/drop_task/complete_task now
  return dropDate (via effectiveDropDate); list_tasks gains a droppedAfter
  filter, mirroring the completionDate/completedAfter pattern.
- create_task/edit_task deferDate/plannedDate/dueDate accept a bare
  YYYY-MM-DD date, resolved to local midnight instead of JS's default of
  UTC midnight — a defer-today task is now Available immediately rather
  than only after the local UTC-offset hour.
- create_task/edit_task can set a task's own sequential (parallel/ordered
  subtasks); surfaced on all task-detail responses.
- move_task's inline response object was missing completionDate/dropDate
  entirely (a pre-existing gap once TaskSummary made completionDate
  required); added both for consistency.
…tasks

A parent-completed child task reports status "complete" but its own
completionDate stays null (only the parent's own completionDate is set),
so any completedAfter scan silently misses it.

Switched every task snippet's completionDate output from task.completionDate
to task.effectiveCompletedDate, which OmniFocus populates from the parent's
completion when the task's own is unset (confirmed live: matches
completionDate exactly for a normally-completed task, so no behaviour
change there). Fixes list_tasks/get_task/create_task/edit_task/drop_task/
complete_task/move_task.
@steveardis

Copy link
Copy Markdown
Owner

Reviewed against current main (738bb19). This is good work and I want it in, but not quite as-is. Details below, most important first.

Merge hazard: this PR's package.json would undo two security-relevant fixes

This is the one thing that must not go wrong. The only merge conflict is in package.json, and this branch's side of it still carries:

Resolving that conflict toward this branch, or hand-merging it, silently reintroduces the CVE and re-breaks the packaging. The resolution is to keep main's package.json wholesale — this PR does not need anything from its own copy.

I verified that path: with main's package.json kept, the merge is clean, tsc --noEmit passes, the unit suite is 157/157, and npm run build still emits all 27 snippets.

One related trap: package-lock.json auto-merges without a conflict but ends up incoherent (root version: 0.1.2 against a package.json saying 0.2.1). Regenerate it with npm install rather than committing what git produces.

droppedAfter and completedAfter return nothing on their own

In list_tasks.js the status filter runs first and, by default, strips complete and dropped. Both new filters then run over what is left, so filter: { completedAfter: "..." } with no status always returns [].

Your integration test gets this right — it passes status: ["dropped"] alongside droppedAfter — so you clearly knew. But neither tool description says so, and the description is what the model reads. An assistant asked "what did I finish this week?" will call completedAfter alone, get an empty array, and report that the user completed nothing.

Please either say so in the description, or have the snippet widen status automatically when completedAfter/droppedAfter is present. I would take either; the second is friendlier but the first is smaller.

completionDate changes meaning rather than gaining a sibling

Switching completionDate to effectiveCompletedDate across get_task, list_tasks, create_task, edit_task, complete_task, drop_task, and move_task redefines a field that already shipped. A subtask the user never completed, whose parent was completed, will now report a completion date.

That is the point of the PR and I think it is the more useful semantic. I would still rather it be explicit — either a separate effectiveCompletionDate field alongside the literal one, or a note in the tool descriptions that the date can come from an ancestor. Silent redefinition is the kind of thing that is very hard to debug from the far side of an MCP boundary. Your call which way; I am not blocking on it.

What I could not verify

I did not run the integration suite — it needs OmniFocus running on macOS against a live database, and I was not going to point it at mine. So the OmniJS semantics this PR depends on are unverified by me: effectiveCompletedDate, effectiveDropDate, and task.sequential.

The containerInfo rewrite deserves specific mention. It assumes task.parentTask is null for a task sitting directly in a project. I believe that holds, because complete_task.js already relies on the same pattern, but if OmniJS instead returns the project's root task there, every top-level project task would come back as containerType: "task" pointing at an id the caller cannot resolve. If you have run the integration suite against a real database on this branch, saying so is enough for me.

Injection review

Clean. Everything reaches the snippets as JSON through args, nothing is concatenated into script source, and parseDateInput uses a strict /^(\d{4})-(\d{2})-(\d{2})$/ match with Number() conversion rather than string building. The one-__ARGS__ contract holds.

Relationship to the other PRs

This PR is a strict superset of #5, and it independently contains everything in #2 (the containerInfo rewrite, completionDate on TaskSummary, and the completedAfter filter). I am closing both in favour of this one so they do not conflict with each other. Flagging one difference: #2 used the literal completionDate where this uses effectiveCompletedDate.

Happy to merge once the two filter descriptions are updated and you have confirmed the integration suite passes on this branch.

…equirement

- Resolve the package.json merge conflict by taking main's copy wholesale, so
  this branch no longer reintroduces vitest ^2.1.0 (CVE-2025-24964), the
  tsc-only build (dropping copy:snippets), or src/snippets/ in files. Regenerate
  package-lock.json with npm install so its root version matches (0.2.1).
- Document on completedAfter/droppedAfter (both the field .describe() and the
  list_tasks tool description) that complete/dropped tasks are excluded by
  default, so each filter returns nothing unless status is widened to include
  "complete"/"dropped".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vishae

vishae commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — all addressed. Pushed as a merge of current main into this branch:

  • package.json merge hazard: resolved by taking main's package.json wholesale, exactly as you suggested — this branch keeps nothing from its own copy. So vitest is back to ^3.2.4 (no more CVE-2025-24964), the build is tsc && npm run copy:snippets, and src/snippets/ is out of files. Regenerated package-lock.json with npm install so its root version is coherent (0.2.1), rather than committing git's auto-merge.
  • Filter descriptions: documented on both completedAfter and droppedAfter — in the field .describe() and in the list_tasks tool description — that complete/dropped tasks are excluded by default, so each filter returns nothing unless status is widened to include "complete"/"dropped". Went with the doc route rather than auto-widening.
  • completionDate semantics: left as the effective date, with the existing note in the tool description that it can come from an ancestor. Happy to switch to a separate effectiveCompletionDate field if you'd rather, but went with the lighter option you flagged as acceptable.

Integration suite passes on this branch against a live OmniFocus database on macOS: 100/100 (28 files), including the effectiveCompletionDate case (parent-completed subtask returns the effective date and is caught by completedAfter), the dropDate/droppedAfter checks, and the containerInfo rewrite. Unit suite is 160/160, tsc --noEmit clean, and npm run build emits all 27 snippets.

On the containerInfo question: confirmed against the live DB — top-level project tasks come back with containerType: "project" (not "task"), so task.parentTask is indeed null for a task sitting directly in a project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants