fix(mcp): allow update wrappers to clear booleans and numerics

copyByJSONTag previously skipped any IsZero value, which made it
impossible for tasks_update / projects_update to flip done from true
to false, reset priority/percent_done to 0, or unarchive a project.

A non-nil pointer src is now the unambiguous "caller supplied this"
signal: dereferenced values are written through even when zero, while
value-typed src fields keep the partial-update semantics. The
affected wrapper fields (Done, IsArchived, IsFavorite, Priority,
PercentDone, RepeatAfter, RepeatMode, BucketID,
CoverImageAttachmentID, ParentProjectID, Position) move to pointer
types so the JSON Schema still marks them optional.
This commit is contained in:
kolaente 2026-05-30 14:49:19 +02:00
parent ecd4d786f7
commit b0bd8ab888
4 changed files with 201 additions and 37 deletions

View File

@ -155,14 +155,14 @@ func setInt64Field(dst any, fieldName string, v int64) error {
// - Field matching is by the first segment of the `json` tag (i.e. // - Field matching is by the first segment of the `json` tag (i.e.
// "title,omitempty" matches "title"). Fields without a json tag (or // "title,omitempty" matches "title"). Fields without a json tag (or
// tagged `json:"-"`) are skipped on both sides. // tagged `json:"-"`) are skipped on both sides.
// - Zero-valued src fields are skipped, so partial updates work // - For value-typed src fields, zero values are skipped so partial
// naturally — only fields the caller actually supplied get propagated. // updates work naturally — only fields the caller actually supplied
// This mirrors the REST update handler's "omitted JSON keys leave the // get propagated. This mirrors the REST update handler's "omitted
// row untouched" behaviour. Wrappers that need to clear a field must // JSON keys leave the row untouched" behaviour.
// model it as a pointer (`*string`, `*int`, etc.) so the zero value // - For pointer-typed src fields, a nil pointer is treated as "absent"
// is distinguishable from "absent". // and skipped. A non-nil pointer is dereferenced and assigned even
// - Pointer src fields are dereferenced. A nil pointer is treated as // when its pointee is the zero value, so wrappers can explicitly set
// "absent" and skipped. // `false` / `0` / `""` by modelling the field as a pointer.
// - Type compatibility: the helper assigns src's value to dst's field // - Type compatibility: the helper assigns src's value to dst's field
// when the types are directly assignable. time.Time / *time.Time work // when the types are directly assignable. time.Time / *time.Time work
// out of the box because time.Time is a struct, not a basic type. // out of the box because time.Time is a struct, not a basic type.
@ -214,17 +214,19 @@ func copyByJSONTag(src, dst any) error {
continue continue
} }
srcVal := sv.Field(i) srcVal := sv.Field(i)
// Skip nil pointers (caller didn't supply the field) and // A non-nil pointer source is treated as "caller explicitly set
// dereference non-nil ones. // this" — even a zero pointee gets propagated so wrappers can
// clear booleans / numerics. Value-typed sources fall back to
// the IsZero heuristic for partial-update semantics.
fromPointer := false
if srcVal.Kind() == reflect.Pointer { if srcVal.Kind() == reflect.Pointer {
if srcVal.IsNil() { if srcVal.IsNil() {
continue continue
} }
srcVal = srcVal.Elem() srcVal = srcVal.Elem()
fromPointer = true
} }
if srcVal.IsZero() { if !fromPointer && srcVal.IsZero() {
// Zero src value → caller didn't populate this field,
// leave dst alone.
continue continue
} }
dstVal := dv.Field(dstIdx) dstVal := dv.Field(dstIdx)
@ -343,14 +345,14 @@ type ProjectUpdateInput struct {
Identifier string `json:"identifier,omitempty" jsonschema:"new short identifier (max 10 chars); omit to leave unchanged"` Identifier string `json:"identifier,omitempty" jsonschema:"new short identifier (max 10 chars); omit to leave unchanged"`
// New hex color (without leading #). Omit to leave unchanged. // New hex color (without leading #). Omit to leave unchanged.
HexColor string `json:"hex_color,omitempty" jsonschema:"new hex color (without leading #); omit to leave unchanged"` HexColor string `json:"hex_color,omitempty" jsonschema:"new hex color (without leading #); omit to leave unchanged"`
// New parent project id. Omit (or zero) to leave unchanged. // New parent project id. Omit to leave unchanged; pass 0 to move to root.
ParentProjectID int64 `json:"parent_project_id,omitempty" jsonschema:"new parent project id; omit or 0 to leave unchanged"` ParentProjectID *int64 `json:"parent_project_id,omitempty" jsonschema:"new parent project id; 0 moves to root, omit to leave unchanged"`
// New ordering position. Omit (or zero) to leave unchanged. // New ordering position. Omit to leave unchanged; pass 0 to reset.
Position float64 `json:"position,omitempty" jsonschema:"new ordering position among siblings; omit or 0 to leave unchanged"` Position *float64 `json:"position,omitempty" jsonschema:"new ordering position among siblings; 0 resets to the start, omit to leave unchanged"`
// Archive state. Omit (or false) to leave un-archived. // Archive state. Omit to leave unchanged.
IsArchived bool `json:"is_archived,omitempty" jsonschema:"set to true to archive, omit or false to leave un-archived"` IsArchived *bool `json:"is_archived,omitempty" jsonschema:"true to archive, false to un-archive, omit to leave unchanged"`
// Favorite state for the caller. Omit (or false) to leave un-favorited. // Favorite state for the caller. Omit to leave unchanged.
IsFavorite bool `json:"is_favorite,omitempty" jsonschema:"set to true to favorite for the caller, omit or false to un-favorite"` IsFavorite *bool `json:"is_favorite,omitempty" jsonschema:"true to favorite for the caller, false to un-favorite, omit to leave unchanged"`
} }
// ApplyTo copies the wrapper fields onto a fresh *models.Project. ID is // ApplyTo copies the wrapper fields onto a fresh *models.Project. ID is
@ -427,6 +429,11 @@ func (in *TaskCreateInput) ApplyTo(dst handler.CObject) error {
// due_date, repeat_after, priority, start_date, end_date, hex_color, // due_date, repeat_after, priority, start_date, end_date, hex_color,
// percent_done, project_id, bucket_id, repeat_mode, cover_image_attachment_id) // percent_done, project_id, bucket_id, repeat_mode, cover_image_attachment_id)
// are exposed. // are exposed.
//
// Booleans and numerics whose zero value carries real meaning ("not done",
// "no priority", "0% complete", "no bucket") are modelled as pointers so
// callers can explicitly clear them. A nil pointer means "omit"; a non-nil
// pointer to the zero value means "set to zero".
type TaskUpdateInput struct { type TaskUpdateInput struct {
// ID of the task to update. Required. // ID of the task to update. Required.
ID int64 `json:"id" jsonschema:"id of the task to update"` ID int64 `json:"id" jsonschema:"id of the task to update"`
@ -436,28 +443,28 @@ type TaskUpdateInput struct {
ProjectID int64 `json:"project_id,omitempty" jsonschema:"move the task to a different project; omit to leave unchanged"` ProjectID int64 `json:"project_id,omitempty" jsonschema:"move the task to a different project; omit to leave unchanged"`
// New description. // New description.
Description string `json:"description,omitempty" jsonschema:"new description; omit to leave unchanged"` Description string `json:"description,omitempty" jsonschema:"new description; omit to leave unchanged"`
// Mark the task as done (true) or undone (false). Defaults to false. // Mark the task as done (true) or undone (false). Omit to leave unchanged.
Done bool `json:"done,omitempty" jsonschema:"true marks the task as done"` Done *bool `json:"done,omitempty" jsonschema:"true marks the task as done, false marks it as not done; omit to leave unchanged"`
// New due date. // New due date.
DueDate time.Time `json:"due_date,omitempty" jsonschema:"new due date as an RFC 3339 timestamp"` DueDate time.Time `json:"due_date,omitempty" jsonschema:"new due date as an RFC 3339 timestamp"`
// New start date. // New start date.
StartDate time.Time `json:"start_date,omitempty" jsonschema:"new start date as an RFC 3339 timestamp"` StartDate time.Time `json:"start_date,omitempty" jsonschema:"new start date as an RFC 3339 timestamp"`
// New end date. // New end date.
EndDate time.Time `json:"end_date,omitempty" jsonschema:"new end date as an RFC 3339 timestamp"` EndDate time.Time `json:"end_date,omitempty" jsonschema:"new end date as an RFC 3339 timestamp"`
// New repeat interval (seconds). // New repeat interval (seconds). Pass 0 to clear.
RepeatAfter int64 `json:"repeat_after,omitempty" jsonschema:"new repeat interval in seconds"` RepeatAfter *int64 `json:"repeat_after,omitempty" jsonschema:"new repeat interval in seconds; 0 clears the repeat"`
// New repeat mode. // New repeat mode. Pass 0 for the after-interval mode.
RepeatMode int `json:"repeat_mode,omitempty" jsonschema:"new repeat mode: 0 = after interval, 1 = monthly, 3 = from current date"` RepeatMode *int `json:"repeat_mode,omitempty" jsonschema:"new repeat mode: 0 = after interval, 1 = monthly, 3 = from current date"`
// New priority. // New priority. Pass 0 to clear.
Priority int64 `json:"priority,omitempty" jsonschema:"new priority value"` Priority *int64 `json:"priority,omitempty" jsonschema:"new priority value; 0 clears the priority"`
// New percent done between 0 and 1. // New percent done between 0 and 1. Pass 0 to reset.
PercentDone float64 `json:"percent_done,omitempty" jsonschema:"new completion percentage between 0 and 1"` PercentDone *float64 `json:"percent_done,omitempty" jsonschema:"new completion percentage between 0 and 1; 0 resets progress"`
// New hex color. // New hex color.
HexColor string `json:"hex_color,omitempty" jsonschema:"new hex color without leading #"` HexColor string `json:"hex_color,omitempty" jsonschema:"new hex color without leading #"`
// New bucket id (move within a kanban view). // New bucket id (move within a kanban view). Pass 0 to detach.
BucketID int64 `json:"bucket_id,omitempty" jsonschema:"new kanban bucket id"` BucketID *int64 `json:"bucket_id,omitempty" jsonschema:"new kanban bucket id; 0 detaches from any bucket"`
// New cover image attachment id. // New cover image attachment id. Pass 0 to clear.
CoverImageAttachmentID int64 `json:"cover_image_attachment_id,omitempty" jsonschema:"new cover image attachment id"` CoverImageAttachmentID *int64 `json:"cover_image_attachment_id,omitempty" jsonschema:"new cover image attachment id; 0 clears the cover"`
} }
// ApplyTo copies the wrapper fields onto a fresh *models.Task. ID is always // ApplyTo copies the wrapper fields onto a fresh *models.Task. ID is always
@ -471,8 +478,8 @@ func (in *TaskUpdateInput) ApplyTo(dst handler.CObject) error {
if err := copyByJSONTag(in, t); err != nil { if err := copyByJSONTag(in, t); err != nil {
return err return err
} }
if in.RepeatMode != 0 { if in.RepeatMode != nil {
t.RepeatMode = models.TaskRepeatMode(in.RepeatMode) t.RepeatMode = models.TaskRepeatMode(*in.RepeatMode)
} }
return nil return nil
} }

View File

@ -227,6 +227,66 @@ func TestCopyByJSONTagSkipsZeroValuesForOptional(t *testing.T) {
assert.InEpsilon(t, 9.9, dst.Position, 0.0001) assert.InEpsilon(t, 9.9, dst.Position, 0.0001)
} }
// TestCopyByJSONTagPointerSrcAllowsZero verifies that pointer-typed src
// fields propagate their pointee even when it's the zero value — this is
// the escape hatch update wrappers use to let callers explicitly set
// `done: false` / `priority: 0` / `is_archived: false`.
func TestCopyByJSONTagPointerSrcAllowsZero(t *testing.T) {
type ptrSrc struct {
Done *bool `json:"done"`
Priority *int64 `json:"priority"`
Position *float64 `json:"position"`
HexColor *string `json:"hex_color"`
}
type valDst struct {
Done bool `json:"done"`
Priority int64 `json:"priority"`
Position float64 `json:"position"`
HexColor string `json:"hex_color"`
}
falseVal := false
zeroInt := int64(0)
zeroFloat := 0.0
empty := ""
src := ptrSrc{
Done: &falseVal,
Priority: &zeroInt,
Position: &zeroFloat,
HexColor: &empty,
}
dst := valDst{
Done: true,
Priority: 5,
Position: 1.5,
HexColor: "ff0000",
}
require.NoError(t, copyByJSONTag(src, &dst))
assert.False(t, dst.Done, "non-nil pointer with false pointee must overwrite true")
assert.Equal(t, int64(0), dst.Priority)
assert.InDelta(t, 0.0, dst.Position, 0.0001)
assert.Empty(t, dst.HexColor)
}
// TestCopyByJSONTagNilPointerSrcSkips verifies that nil pointer src fields
// are treated as "absent" — the dst keeps whatever it had.
func TestCopyByJSONTagNilPointerSrcSkips(t *testing.T) {
type ptrSrc struct {
Done *bool `json:"done"`
Priority *int64 `json:"priority"`
}
type valDst struct {
Done bool `json:"done"`
Priority int64 `json:"priority"`
}
src := ptrSrc{} // both nil
dst := valDst{Done: true, Priority: 7}
require.NoError(t, copyByJSONTag(src, &dst))
assert.True(t, dst.Done, "nil pointer must not overwrite")
assert.Equal(t, int64(7), dst.Priority)
}
type srcWithPointers struct { type srcWithPointers struct {
Title *string `json:"title"` Title *string `json:"title"`
Due *time.Time `json:"due"` Due *time.Time `json:"due"`
@ -267,3 +327,41 @@ func TestCopyByJSONTagTimeValue(t *testing.T) {
require.NoError(t, copyByJSONTag(src, &dst)) require.NoError(t, copyByJSONTag(src, &dst))
assert.Equal(t, now, dst.Due) assert.Equal(t, now, dst.Due)
} }
// TestProjectUpdateInputClearsBooleans verifies that a wrapper carrying
// `is_archived: false` (via a non-nil *bool) actually clears IsArchived
// on the destination Project, even when the dst started with IsArchived=true.
// This guards the regression flagged in PR review: prior to the pointer-source
// fix, all zero values were silently dropped by copyByJSONTag.
func TestProjectUpdateInputClearsBooleans(t *testing.T) {
falseVal := false
in := &ProjectUpdateInput{ID: 1, IsArchived: &falseVal, IsFavorite: &falseVal}
p := &models.Project{ID: 1, IsArchived: true, IsFavorite: true}
require.NoError(t, in.ApplyTo(p))
assert.False(t, p.IsArchived, "IsArchived must clear when explicitly set to false")
assert.False(t, p.IsFavorite, "IsFavorite must clear when explicitly set to false")
}
// TestTaskUpdateInputClearsBoolsAndZeros mirrors the project test for tasks
// — done can flip to false, priority can drop to 0, percent_done resets.
func TestTaskUpdateInputClearsBoolsAndZeros(t *testing.T) {
falseVal := false
zeroPriority := int64(0)
zeroPercent := 0.0
in := &TaskUpdateInput{
ID: 1,
Done: &falseVal,
Priority: &zeroPriority,
PercentDone: &zeroPercent,
}
tk := &models.Task{
ID: 1,
Done: true,
Priority: 5,
PercentDone: 0.75,
}
require.NoError(t, in.ApplyTo(tk))
assert.False(t, tk.Done)
assert.Equal(t, int64(0), tk.Priority)
assert.InDelta(t, 0.0, tk.PercentDone, 0.0001)
}

View File

@ -290,6 +290,35 @@ func TestMCP_Projects_Update(t *testing.T) {
assert.Equal(t, "Updated description", project["description"]) assert.Equal(t, "Updated description", project["description"])
} }
// TestMCP_Projects_UpdateClearsArchived exercises the pointer-source path
// of copyByJSONTag: an explicit `is_archived: false` must un-archive a
// project that was previously archived.
func TestMCP_Projects_UpdateClearsArchived(t *testing.T) {
c := newMCPClient(t, mcpFullProjectsToken)
createResult := c.callTool("projects_create", map[string]any{
"title": "mcp project to un-archive",
"is_archived": true,
})
require.NotContains(t, createResult, "isError")
var created map[string]any
require.NoError(t, json.Unmarshal([]byte(toolResultText(t, createResult)), &created))
pid := int64(created["id"].(float64))
require.True(t, created["is_archived"].(bool), "project should have been created archived")
updateResult := c.callTool("projects_update", map[string]any{
"id": pid,
"is_archived": false,
})
require.NotContains(t, updateResult, "isError", "update errored: %v", updateResult)
readResult := c.callTool("projects_read_one", map[string]any{"id": pid})
require.NotContains(t, readResult, "isError")
var project map[string]any
require.NoError(t, json.Unmarshal([]byte(toolResultText(t, readResult)), &project))
assert.False(t, project["is_archived"].(bool), "is_archived must be false after explicit clear")
}
func TestMCP_Projects_Delete(t *testing.T) { func TestMCP_Projects_Delete(t *testing.T) {
c := newMCPClient(t, mcpFullProjectsToken) c := newMCPClient(t, mcpFullProjectsToken)

View File

@ -106,6 +106,36 @@ func TestMCP_Tasks_Update(t *testing.T) {
assert.Equal(t, "Updated description", task["description"]) assert.Equal(t, "Updated description", task["description"])
} }
// TestMCP_Tasks_UpdateClearsDone exercises the pointer-source path of
// copyByJSONTag: a `done: false` explicitly supplied through the JSON
// args must flip a task from done back to undone.
func TestMCP_Tasks_UpdateClearsDone(t *testing.T) {
c := newMCPClient(t, mcpFullProjectsToken)
createResult := c.callTool("tasks_create", map[string]any{
"title": "mcp task to undo",
"project_id": 1,
"done": true,
})
require.NotContains(t, createResult, "isError")
var created map[string]any
require.NoError(t, json.Unmarshal([]byte(toolResultText(t, createResult)), &created))
tid := int64(created["id"].(float64))
require.True(t, created["done"].(bool), "task should have been created in done state")
updateResult := c.callTool("tasks_update", map[string]any{
"id": tid,
"done": false,
})
require.NotContains(t, updateResult, "isError", "update errored: %v", updateResult)
readResult := c.callTool("tasks_read_one", map[string]any{"id": tid})
require.NotContains(t, readResult, "isError")
var task map[string]any
require.NoError(t, json.Unmarshal([]byte(toolResultText(t, readResult)), &task))
assert.False(t, task["done"].(bool), "done must be false after explicit clear")
}
func TestMCP_Tasks_Delete(t *testing.T) { func TestMCP_Tasks_Delete(t *testing.T) {
c := newMCPClient(t, mcpFullProjectsToken) c := newMCPClient(t, mcpFullProjectsToken)