fix: task date tools expose only start and due dates, matching the UI - EXO-88465 - #649
Conversation
|
azayati
left a comment
There was a problem hiding this comment.
AI review — Round #1
Backport PR. Per ai-review-and-merge.md (eXip backports are rule 5's second face) the scope is diff-equivalence with the validated FB state, plus full review of any conflict-resolution hunk — with the diff read on its merits too, since this targets develop. Findings are tagged [inherited] (a property of the PO-validated FB commit, carried faithfully) or [backport-blocking].
Diff-equivalence: verified. 57683b16 diffed against the SHA in its own (cherry picked from …) trailer, e97446880 — byte-identical, matching the body's table. One commit, no conflict-resolution hunk, no session URL in the message.
🟡 [backport-blocking, body only] The PR body attributes this diff's behavioural change to a PR that is already on develop — so a reviewer reading the body under-reviews the one thing worth reviewing
The body's task table says "task#620 (the patch-semantics fix itself) is already on develop", and the diff summary describes only "the end_date parameter removed … wording aligned with patch semantics". But setTaskDates on develop today is the unguarded three-setter form:
// origin/develop:services/src/main/java/io/meeds/task/mcp/TaskMcpTool.java
public TaskModel setTaskDates(long taskId, String startDate, String endDate, String dueDate) … {
TaskDto task = getEditableTask(taskId);
task.setStartDate(toDate(startDate)); // no isNotBlank guard anywhere in this method
task.setEndDate(toDate(endDate));
task.setDueDate(toDate(dueDate));The only three isNotBlank occurrences in develop's TaskMcpTool are at :284, :639 and :645 — none of them in setTaskDates. So this PR is what changes set_task_dates from replace semantics to patch semantics on develop, and that write-semantics change — not the parameter removal — is the substantive part of the diff. Combined with "N2 proposed", the body invites a lighter review than the diff deserves. (The change itself is correct and well tested; see verified conform. This finding is about the body, not the code.)
Fix: correct the table's Note and the diff summary to say that this PR carries the patch-semantics change itself, and keep whatever task#620 actually delivered out of this PR's description.
🟡 [inherited] After this change there is no longer any way to clear a task date through the AI tools
ai-tool-definitions.json — the description changes meaning, not just wording:
-"description": "Set or reschedule the start, due and/or end dates of an existing task. Pass a date as a string; omit a date to clear it."
+"description": "Set or reschedule the start and/or due dates of an existing task. Pass a date as a string; omitting a date leaves it unchanged."Clearing a date was an advertised capability of this tool, and omission was the way to reach it. With the new guard a blank never reaches the setter, and set_task_dates is the only date-writing update tool in the whole definition file — the other 24 task tools are title, description, status, priority, labels, coworkers, comments, favorite, complete/reopen and reads (verified against the full "name": list at head). So a user who asks EVA to remove a due date now has no path at all, where before they had one that also wiped the other dates.
This is the deliberate trade-off EXO-88465 asked for, so it is not an implementation defect — but the replacement was not shipped with it.
Fix: a product decision rather than a code one — confirm with the PO whether "clear a date" should come back as an explicit affordance (a clear_start_date/clear_due_date tool, or a documented sentinel such as "none" handled in setTaskDates), or is deliberately dropped. If dropped, the tool description saying so outright would stop an agent retrying omission to clear.
🟢 Nit — the reply still advertises a date the tools can no longer set
@JsonProperty("end_date")
private String endDate;The three input schemas drop end_date, but the response model keeps it, so every task an agent creates or reschedules comes back carrying an end_date field that is now always null for MCP-created tasks and that no tool can write. Harmless, but it is the kind of asymmetry that makes a model try to set it again.
Fix: either drop end_date from TaskModel as well, or leave it and note in the field's javadoc that it is read-only for the AI surface — whichever matches how much the field is still relied on by existing consumers of these tool replies.
Verified conform
The premise of the change holds. I checked it rather than taking the commit message's word: TaskFormDatePickers.vue — the Task drawer's date component — binds pickers to startDate (:23-29) and dueDate (:78-84) only; the one occurrence of endDate in that file is a prop-name default (:145 default: () => 'endDate'), not a rendered date field. So the UI genuinely surfaces two dates, and calling the third vestigial on the AI surface is accurate.
The tests pin the fix in both directions and are mutation-sensitive. setTaskDatesShouldNotOverwriteOmittedDates and setTaskDatesWithOnlyDueDateShouldUpdateDueNotStartAndNeverEnd each assert the written date and verify(task, never()).setDueDate(any()) / never()).setStartDate(any()), plus never()).setEndDate(any()) in both — reverting the guard fails them on the never() assertion, so they are evidence rather than decoration.
No leftover and no collateral caller. end_date is gone from all three input schemas, with no stray occurrence left in services/src/main/resources (the only other hits in the repo are the unrelated frappegantt.js bundle and TaskModel, above). createTaskInProject, createPersonalTask and setTaskDates have no caller outside TaskMcpTool/its test, so the three signature changes cannot have broken a REST or service path — which is also why the build stayed green on a signature change.
Scope: no REST, DAO, entity, Liquibase or ACL surface is touched, and no tool is added — the ACL path (getEditableTask → userAcl.hasEditPermission) is untouched and still covered by the unchanged permission test.
What this PR does well
The backport is one clean pick with its source SHA in the trailer, which makes the equivalence check a single command. On the content: the patch-semantics guard is applied to both dates symmetrically rather than only to the one the bug report named, the schema description was rewritten to match the new semantics instead of being left to contradict them, and the three javadoc blocks added on the changed methods state the why ("dates mirror the Task UI") where a future reader will meet it — so the next person who wonders where end_date went does not have to find this task.
Classification
N2, confirming the body's proposal. I considered the ai-review-and-merge.md §2 trigger "new MCP tool definitions (new attack surface)" and it does not fire: no tool is added, three existing input schemas are narrowed, which reduces rather than extends the surface. No ACL, schema, trust-boundary or Kernel↔Spring hunk. Per §1 an N2 needs the AI review's findings resolved or explicitly waived, plus one human ack — and per §5 the approver should know it is N2; it must not be auto-merged on AI review alone, and the approver must not be the author.
🤖 Generated with Claude Code
|
Round 1: you are right on the body — I checked |
azayati
left a comment
There was a problem hiding this comment.
AI review — Round #2 (closing round)
Head unchanged at 57683b16 — the round-1 response was a body correction, no new commit. Nothing new to fix.
| # | Round 1 finding | Status |
|---|---|---|
| 1 | 🟡 Body attributed the patch-semantics change to task#620, already on develop |
✅ Fixed |
| 2 | 🟡 [inherited] No AI tool can clear a task date any more | ➖ Accepted — PO decision, noted for follow-up |
| 3 | 🟢 [inherited] TaskModel.end_date still in replies while no tool writes it |
➖ Accepted — noted for follow-up |
1 — the correction is accurate on both halves, checked independently rather than taken from the reply. develop's setTaskDates still contains zero isNotBlank occurrences, so the guards genuinely arrive with this pick; and gh pr view 620 reports "baseRefName": "feature/ai-contribution", so task#620 did merge on the FB and not on develop, exactly as the corrected table note now says. The diff summary also now names the semantics change (replace → patch) rather than describing the PR as a parameter removal — which is the part that decides how hard a reviewer looks.
2 and 3 — recorded as inherited trade-offs with an owner. Both are properties of the PO-validated FB commit rather than defects the backport introduced, and the body now carries them explicitly, with the clear-a-date affordance routed as a PO decision. That is the right disposition for a mechanical backport; this round does not re-open either.
Diff-equivalence re-checked at the unchanged head. 57683b16 is still byte-identical to its cherry-pick source e97446880, and the branch is still one commit over develop. Since no commit was added, everything Round #1 verified in the code stands unchanged: the UI premise (TaskFormDatePickers.vue binds pickers to startDate and dueDate only, its single endDate being a prop-name default), the two new tests asserting both the write and the never() on the untouched dates, no leftover end_date in the tool definitions, and no caller of the three changed methods outside TaskMcpTool and its test.
Nothing outstanding from the AI review side: the one actionable item is closed, and the two inherited ones are accepted with a follow-up owner.
Classification: N2, unchanged — the code is byte-identical to what Round #1 classified, and the ai-review-and-merge.md §2 "new MCP tool definitions" trigger still does not fire (no tool added; three existing input schemas narrowed). This PR targets develop, a trusted lineage: it must be approved by an Architect/Senior Developer who knows it is N2, not auto-merged on AI review alone, and the approver must not be the author (§5).
🤖 Generated with Claude Code
azayati
left a comment
There was a problem hiding this comment.
AI review — Round #3 (final) — close-out
Independent pass by a fresh reviewer that had not seen the previous rounds, at head 57683b16. This PR is clean — nothing to fix in it. One finding came back, and it is a platform property of mcp-server that this diff neither introduces nor can fix; it is carried below because it changes what a release note must say.
| # | Finding | Status |
|---|---|---|
| 1 | 🟡 Body attributed the patch-semantics change to task#620 (R1) | ✅ Fixed (R2) |
| 2 | 🟡 [inherited] No AI tool can clear a task date (R1) | ➖ Accepted — PO decision, unchanged |
| 3 | 🟢 [inherited] TaskModel.end_date still in replies (R1) |
➖ Accepted |
| 4 | 🟡 [inherited, platform] Upgraded instances keep serving the old tool schema | 🆕 Non-blocking — release note + corpus, owners named below |
🟡 [inherited — mcp-server, not introduced by this pick] On any instance that has already served a tool list, the old set_task_dates schema keeps shadowing the new one
The metadata the MCP client sees does not come from the ai-tool-definitions.json this PR edits, once a definition of the same name has been persisted. I verified this myself at mcp-server origin/develop (current tip fd68fb2), McpServerToolService.retrieveToolDefinitions():
.map(toolDefinition -> savedDefinitions.stream()
.filter(t -> t.getName().equals(toolDefinition.getName()))
.findFirst()
.orElse(toolDefinition)) // the SAVED one wins, wholesale
.toList();
saveToolsContent(McpToolUtils.toJsonStringBase64(new ToolDefinitionMethods(toolDefinitionList)));The saved entry replaces the classpath one entirely, and SimpleToolDefinition carries description and input_schema as fields of its own — so an edit to either is ignored for any tool name already in the blob. The blob is then rewritten unconditionally, on the first getToolDefinitions() after every boot. The two reset paths do not fire on an upgrade: meeds.mcp.tools.forceReimport is @Value("${meeds.mcp.tools.forceReimport:false}"), and TOOLS_KEY is "AI_AGENT_TOOL_DEFINITIONS_v13", whose only appearance in history is the addon move (5d4ce5d) — it has not moved since.
Consequence for this PR specifically: on an upgraded (not fresh) instance the model is still told set_task_dates takes end_date and still reads "omit a date to clear it", while the code now leaves an omitted date unchanged and has no endDate parameter. A model following the stale description to clear a due date silently does nothing — the exact confusion EXO-88465 set out to remove. One sub-claim deliberately left as a hypothesis: what Spring AI's MethodToolCallback does with an argument that has no matching parameter (silent drop vs. error) could not be verified — no spring-ai-* jar was reachable on this machine. Whoever writes the release note should settle that before describing the symptom.
This bites every tool-definition edit in every addon, identically on feature/ai-contribution. It is not a reason to hold this PR.
Fix: nothing in this diff. Two actions with different owners — (a) Ops / release note for the develop composition: existing instances need meeds.mcp.tools.forceReimport=true once (or a reset of the AI_AGENT_TOOL_DEFINITIONS_v13 setting) for any new tool schema to reach clients; (b) Architect, mcp-server side: whether re-import should refresh input_schema/description from the classpath while preserving only the user-set disabled / require_approval overrides is a design call, not this PR's.
Verified conform
Backport mechanics, established more strongly than by diffing. The three touched blobs at head are the same git objects as at e97446880 (git rev-parse 57683b16:<path> equals git rev-parse e97446880:<path> for all three files), and develop and the FB parent had not diverged on them at all — so diff-equivalence with the PO-validated state is exact, with zero conflict-resolution hunks. One commit over develop; git diff origin/develop 57683b16 equals the commit's own patch, so no stowaway change. Title carries EXO-88465, the -x provenance line and the Co-Authored-By trailer are present, and there is no session URL in the message or the body.
The guard, on every input shape. toDate is if (StringUtils.isBlank(date)) return null; — so null, "" and " " are all caught by isNotBlank before it is reached, collapsing to "leave unchanged" exactly as the new description says. A malformed date does not silently no-op: ISO8601.parse swallows ParseException and returns null, so toDate("tomorrow") NPEs and surfaces as a tool-execution error. Ugly against backend-spring.md §5, but reached the same way before this diff and still reached from createProjectInSpace — pre-existing, not a finding here.
Creation paths stay correct unguarded: createTaskInProject and createPersonalTask mutate a freshly built Task, where "replace with null" and "leave unchanged" coincide.
Every reader of endDate swept, and none breaks. getTaskWorkLoad (falls back to endDate when dueDate is null), TaskLoggingListener / TaskSavedListener workplan-change detection, the analytics endDate statistic, and TaskUtil.saveTaskField (the legacy workPlan REST field — now the only remaining writer). All degrade to "no workload / no event / no date", none throws, and the two change-detection sites are || conditions still firing on a startDate change. The genuinely dangerous pair, TaskQuery.setStartDate / setEndDate (a date-range filter over the endDate column), has no caller in main sources. No Gantt view exists in this repo.
The three schemas. end_date is gone from the whole file — no occurrence of end_date or endDate remains in ai-tool-definitions.json. Property order still matches each Java signature positionally in all three tools, so the removal desynchronised nothing; no required array ever named end_date; destructiveHint: true on set_task_dates still matches its siblings.
Tests. The meaningful mutant is removing the two isNotBlank guards while keeping the 3-parameter signature (a full revert would not compile). Under it, toDate(null) returns null, setDueDate(null) is invoked, and verify(task, never()).setDueDate(any()) fails — Mockito's any() matches null. Both pins hold, symmetrically. The one uncovered input shape is a malformed non-blank date, whose behaviour this diff does not change.
Cross-repo coherence: end_date appears in none of ai, meeds, meeds-qa-ui, mcp-server, notes, content. The ai repo's chat labels reference these tool names but interpolate only {project_id} / {title} / {description}, and carry no set_task_dates label — no UI string describes a parameter that no longer exists. Self-contained in task; no release-order dependency.
Ledger items 2 and 3 re-derived, and the round-2 classification agreed with. setTaskDates is verifiably the only date writer among the MCP tools and blank now means unchanged, so nothing in the MCP surface can null a start or due date — deliberate FB content, replacement affordance is a functional-scope call, correctly ➖ and routed to the PO. Item 3 confirmed read-only and correctly 🟢.
Close-out by-products (dev-lifecycle.md §3b step 5)
- Corpus gap to capture:
domains/mcp-server.md§14 documents the base64 blob and says a change to the shape ofSimpleToolDefinitionneeds a key bump, but says nothing about a saved definition's content shadowing the classpath JSON, nor aboutmeeds.mcp.tools.forceReimport. That is finding 4's general form and is worth one bullet — it silently defeats tool-definition edits org-wide. - Domain-doc refresh:
domains/task.md'sTaskMcpToolmethod list predatessetTaskDates,setTaskPriorityandcompleteTask/reopenTask, and the doc carries noVerifiedstamp. Stale independently of this PR, which is whyKnowledge: noneis defensible here — the debt belongs to the FB delivery, not to a mechanical pick.
What this delivery does well
The pick is as clean as a backport gets — identical blobs, zero divergence on the touched files, -x provenance, Knowledge: line, no session URL. The guard is placed on the caller side rather than inside toDate, so no other tool's semantics move with it, and the schema description was rewritten in the same commit so the contract the model reads matches the code — the failure mode most such PRs ship with is precisely a description left saying the old thing. The two tests pin the asymmetric case rather than the happy path and both assert never() on setEndDate as a re-introduction guard. And the round-1 body correction was a real one: develop's setTaskDates genuinely has no guard, so the original framing understated the diff.
Close-out
All findings from previous rounds are resolved or explicitly accepted with an owner, and the fresh pass found nothing to fix in this PR. Nothing outstanding from the AI review side. Two items remain for humans, neither blocking the merge: the PO decision on whether a date-clearing affordance returns (item 2), and the Ops/Architect actions behind item 4.
Classification: N2, confirmed independently. The §2 trigger "new MCP tool definitions (new attack surface)" does not fire on wording or rationale — no tool is added and the surface shrinks. No ACL (getEditableTask untouched), no schema/Liquibase/JPA, no Kernel↔Spring bridge, no client-controlled input newly reaching a trusted context. Targeting develop, a trusted lineage, the N2 routing applies: 1 human ack, author ≠ approver, and the approver should know it is N2 — not auto-merged on AI review alone.
🤖 Generated with Claude Code
|
Round 3: nothing to change in the diff, agreed. Finding 4 is now a Release note section in the body — upgraded instances need |
Backport onto
developof EXO-88465 fromfeature/ai-contribution: the task date tools expose onlystart_dateanddue_date, the two dates the Tasks UI shows, so asking EVA for an end date no longer writes the hiddenendDate. One-xcherry-pick, applied clean on a freshorigin/developworktree (b7e5de042).Task, identified by its cherry-pick
57683b164e97446880(task#627)develop'ssetTaskDateshas noisNotBlankguard, and task#620 was merged on the FB, not ondevelop(corrected after round 1)Diff against
develop(3 files, +84/−25)TaskMcpTool.java,ai-tool-definitions.json(theend_dateparameter removed fromset_task_dates,create_task_in_project,create_personal_task;setTaskDatesgains theisNotBlankguards that turn replace semantics into patch semantics — an omitted date is left unchanged instead of being wiped; the description says so),TaskMcpToolTest.java. No REST, DAO, entity or Liquibase change; no new tool.Build
mvn clean verifyon the branch: BUILD SUCCESS, 221 tests, 0 failures.Review round 1 (azayati, 2026-09-11)
develop.set_task_datesis the only date writer). Deliberate trade-off of EXO-88465 on the FB; whether a clear affordance comes back is a PO decision, noted for follow-up, not changed here.TaskModel.end_datestill in replies while no tool writes it. FB content, noted for follow-up.Release note (round 3, platform property — not this diff)
On an instance that already served a tool list, mcp-server's saved tool definitions shadow the classpath
ai-tool-definitions.jsonwholesale (McpServerToolService.retrieveToolDefinitions), so an upgraded instance keeps telling the model thatset_task_datestakesend_dateand that omission clears a date. To get the new schema to clients once, setmeeds.mcp.tools.forceReimport=truefor one boot (or reset theAI_AGENT_TOOL_DEFINITIONS_v13setting). Fresh instances are unaffected. Applies to every tool-definition edit in every addon, on the FB as well; corpus capture (domains/mcp-server.md§14) and the mcp-server design question are for the Architects, not this PR.Classification
N2 proposed (recall-first): existing MCP tool parameters change shape; no new tool, no ACL or schema surface.
Knowledge: none — mechanical backport of a PO-validated FB commit.
🤖 Generated with Claude Code