diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-08-14 08:12:29 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-08-14 08:12:29 +0000 |
| commit | b55cfbbd433bed6035dfa228ee700e2cca060ca4 (patch) | |
| tree | 53e9575785fdce504fa9ee1184dd057ed3fc7676 | |
| parent | dcfb7204d654c98657b0638a569e8ec69bb5d852 (diff) | |
Fix integration papercuts found in cross-source review
getAtomDetails's gtasks case was a stub returning a hardcoded "Google
Task" title, so every gtask completed via the web Tasks/Timeline tab
or the Agent API logged into completed-tasks history with no real
title or due date -- now calls findGoogleTask like every other gtasks
call site already does. Gtasks completion also skipped cache
invalidation in two call sites (HandleCompleteAtom, the Agent API's
handleAgentTaskToggle) that already had it for trello; both now
invalidate CacheKeyGoogleTasks the same way the widget handlers do.
HandleTaskDetailPage (the widget deep-link fallback page) hand
-duplicated loadTaskDetailData's lookup instead of calling it, so it
never got this session's earlier gtasks-detail fix -- now delegates.
Also deleted PlanToEatAPI.GetRecipes, dead code since its introduction
("for Phase 2," never called).
Adds a running .agent/critiques.md tracking structural/process
critiques surfaced in conversation, separate from a concrete plan.
| -rw-r--r-- | .agent/critiques.md | 78 | ||||
| -rw-r--r-- | internal/api/interfaces.go | 1 | ||||
| -rw-r--r-- | internal/api/plantoeat.go | 5 | ||||
| -rw-r--r-- | internal/handlers/agent.go | 10 | ||||
| -rw-r--r-- | internal/handlers/agent_test.go | 41 | ||||
| -rw-r--r-- | internal/handlers/handlers.go | 34 | ||||
| -rw-r--r-- | internal/handlers/handlers_test.go | 80 |
7 files changed, 215 insertions, 34 deletions
diff --git a/.agent/critiques.md b/.agent/critiques.md new file mode 100644 index 0000000..a507cbd --- /dev/null +++ b/.agent/critiques.md @@ -0,0 +1,78 @@ +# Critiques (running list) + +Working notes on structural/design weaknesses, gathered as they come up in +conversation rather than invented in the abstract. Not a plan — a list to +react to and prioritize before anything here gets acted on. + +## Data model + +1. **`native_tasks` is one flat table doing ~4 jobs.** A task, a + chain-membership, a bucket-membership, and a recurrence-series are all + the same row, distinguished only by which subset of bolt-on columns + (`chain_id/position/unlocked`, `bucket_id/state/last_active_at`, + `recurrence_freq/interval/weekdays/series_id/override`) are non-empty. + Every new feature adds more nullable-in-practice columns to the same + row; every query has to reason about which columns are "live" for a + given row's shape. Cost grows linearly with feature count. + +2. **No real task-dependency graph.** "Blocking" / "depends on" only exist + as (a) a chain's implicit position ordering, or (b) free-text notes a + human has to read and self-enforce. There's no way to express "task B + can't start until task A" when A isn't B's chain predecessor, without + falling back to prose. This is the literal gap hit building the Pelagic + Marine Electronics project (5 real dependency edges, none enforceable). + +3. **Chains and projects are welded 1:1.** `CreateChain` always creates its + own backing project — no chain-within-an-existing-project, no + multiple-chains-sharing-a-project. This is why one business plan with 7 + phases produced 7 separate entries in the project list. Looks like a + modeling shortcut, not a deliberate choice. + +4. **Three incompatible "shapes" of repeat/pooled work.** Chains (WIP-1, + ordered, position-based), buckets (pick-N pool, cycle-based, + staleness-ordered), and recurrence (single-task series, date-anchored) + solve overlapping problems with three separate non-composable + mechanisms and three separate column sets on the same table. A task + can't be "a bucket item that's also gated behind another task" — none + of the three concepts compose. + +5. **Labels have no referential integrity.** `labels(name, color)` is + color metadata only; actual label assignment is a raw JSON array on + `native_tasks` with no FK to `labels.name`. A typo in a label string + silently creates an new, uncolored "label" rather than erroring or + reusing the existing one. + +## Process / instruction-file cruft (doot-scoped) + +6. **`.agent/config.md`'s "ULTRA-STRICT ROOT SAFETY PROTOCOL"** (wait for + explicit "GO" before any system-changing call) contradicts how this + project actually runs — confirmed stale 2026-08-14. Pending: fold into + whatever the workspace-wide instruction-file cleanup lands on, rather + than patch in isolation. + +7. **`.agent/worklog.md`** references issues (#66-73) that don't match any + real recent work — leftover scaffolding, not maintained. Confirmed + stale 2026-08-14. Same pending treatment as #6. + +## Integrations + +Reviewed 2026-08-14. Five immediate papercuts found and fixed same day: +`getAtomDetails`'s gtasks case was a stub (logged every completed Google +Task as literally "Google Task" with no due date); gtasks completion via +the web Tasks/Timeline tab and the Agent API skipped cache invalidation +(trello had it, gtasks didn't, in two call sites); `HandleTaskDetailPage` +hand-duplicated `loadTaskDetailData`'s lookup instead of calling it, so it +never got the earlier gtasks-detail fix; `PlanToEatAPI.GetRecipes` was +dead code (own comment: "for Phase 2," never called) — deleted. + +Long-term questions surfaced, not decided: +- Should `GoogleTasksAPI` grow real create/due-date methods (the REST API + supports both; the interface just never exposed them)? +- Should the ~13 copy-pasted `switch source` dispatch blocks collapse into + one shared resolver? Not urgent at 3 actionable sources. +- The ad hoc `findGoogleTask`/`findCard` linear-scan-by-ID lookups are + copy-pasted inline in ~5 places instead of reused — style issue at + current (personal-scale) data volumes, not a performance one. +- Read-path caching is already well-unified (generic `CacheFetcher[T]`) — + don't over-correct that side, the inconsistency was only ever on + write/invalidation. diff --git a/internal/api/interfaces.go b/internal/api/interfaces.go index e764130..02e0cb5 100644 --- a/internal/api/interfaces.go +++ b/internal/api/interfaces.go @@ -21,7 +21,6 @@ type TrelloAPI interface { type PlanToEatAPI interface { GetUpcomingMeals(ctx context.Context, days int) ([]models.Meal, error) GetShoppingList(ctx context.Context) ([]models.ShoppingItem, error) - GetRecipes(ctx context.Context) error } // GoogleCalendarAPI defines the interface for Google Calendar operations diff --git a/internal/api/plantoeat.go b/internal/api/plantoeat.go index 770987a..edd494a 100644 --- a/internal/api/plantoeat.go +++ b/internal/api/plantoeat.go @@ -184,11 +184,6 @@ func normalizeMealType(mealType string) string { } } -// GetRecipes fetches recipes (for Phase 2) -func (c *PlanToEatClient) GetRecipes(ctx context.Context) error { - return fmt.Errorf("not implemented yet") -} - // GetShoppingList fetches the shopping list by scraping the web interface // Requires a valid session cookie set via SetSessionCookie func (c *PlanToEatClient) GetShoppingList(ctx context.Context) ([]models.ShoppingItem, error) { diff --git a/internal/handlers/agent.go b/internal/handlers/agent.go index 151520b..577ff87 100644 --- a/internal/handlers/agent.go +++ b/internal/handlers/agent.go @@ -450,12 +450,18 @@ func (h *Handler) handleAgentTaskToggle(w http.ResponseWriter, r *http.Request, if complete { title, dueDate := h.getAtomDetails(id, source) _ = h.store.SaveCompletedTask(source, id, title, dueDate) - if source == "trello" { + switch source { + case "trello": _ = h.store.DeleteCard(id) + case "gtasks": + _ = h.store.InvalidateCache(store.CacheKeyGoogleTasks) } } else { - if source == "trello" { + switch source { + case "trello": _ = h.store.InvalidateCache(store.CacheKeyTrelloBoards) + case "gtasks": + _ = h.store.InvalidateCache(store.CacheKeyGoogleTasks) } } diff --git a/internal/handlers/agent_test.go b/internal/handlers/agent_test.go index 2893a13..ea661b1 100644 --- a/internal/handlers/agent_test.go +++ b/internal/handlers/agent_test.go @@ -13,6 +13,7 @@ import ( "task-dashboard/internal/config" "task-dashboard/internal/models" + "task-dashboard/internal/store" ) func TestHandleAgentAuthRequest(t *testing.T) { @@ -833,6 +834,46 @@ func TestHandleAgentTaskWriteOperations(t *testing.T) { }) } +func TestHandleAgentTaskComplete_Gtasks_InvalidatesCache(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + + if err := db.SaveGoogleTasks([]models.GoogleTask{ + {ID: "g1", Title: "Renew passport", ListID: "list-a", UpdatedAt: time.Now()}, + }); err != nil { + t.Fatal(err) + } + if err := db.UpdateCacheMetadata(store.CacheKeyGoogleTasks, 60); err != nil { + t.Fatal(err) + } + + h := &Handler{store: db, googleTasksClient: &mockGoogleTasksClient{}, config: &config.Config{}} + + session := &models.AgentSession{RequestToken: "rt", AgentName: "A", AgentID: "a-uuid", ExpiresAt: time.Now().Add(5 * time.Minute)} + db.CreateAgentSession(session) + db.ApproveAgentSession("rt", "st", time.Now().Add(time.Hour)) + + req := httptest.NewRequest(http.MethodPost, "/agent/tasks/g1/complete?source=gtasks&listId=list-a", nil) + req = req.WithContext(context.WithValue(req.Context(), agentSessionContextKey, session)) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("id", "g1") + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + w := httptest.NewRecorder() + + h.HandleAgentTaskComplete(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", w.Code, w.Body.String()) + } + meta, err := db.GetCacheMetadata(store.CacheKeyGoogleTasks) + if err != nil { + t.Fatal(err) + } + if meta != nil { + t.Error("expected gtasks cache metadata to be invalidated after agent-api completion, but it still exists") + } +} + func TestHandleAgentCreateOperations(t *testing.T) { db, cleanup := setupTestDB(t) defer cleanup() diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 7220474..aedbd11 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -667,6 +667,8 @@ func (h *Handler) handleAtomToggle(w http.ResponseWriter, r *http.Request, compl switch source { case "trello": _ = h.store.DeleteCard(id) + case "gtasks": + _ = h.store.InvalidateCache(store.CacheKeyGoogleTasks) case "doot": // native tasks stay in DB marked completed; no cache to invalidate } @@ -712,7 +714,9 @@ func (h *Handler) getAtomDetails(id, source string) (string, *time.Time) { } } case "gtasks": - return "Google Task", nil + if t, ok := h.findGoogleTask(id); ok { + return t.Title, t.DueDate + } } return "Task", nil } @@ -906,30 +910,8 @@ func (h *Handler) HandleTaskDetailPage(w http.ResponseWriter, r *http.Request) { return } - var title, description string - switch source { - case "doot": - if tasks, err := h.store.GetNativeTasks(); err == nil { - for _, t := range tasks { - if t.ID == id { - title, description = t.Content, t.Description - break - } - } - } - case "trello": - if boards, err := h.store.GetBoards(); err == nil { - for _, b := range boards { - for _, c := range b.Cards { - if c.ID == id { - title, description = c.Name, c.Description - break - } - } - } - } - } - + detail := h.loadTaskDetailData(id, source) + title := detail.Title if title == "" { title = "Task" } @@ -941,7 +923,7 @@ func (h *Handler) HandleTaskDetailPage(w http.ResponseWriter, r *http.Request) { Description string CSRFToken string Saved bool - }{title, id, source, description, auth.GetCSRFTokenFromContext(r.Context()), r.URL.Query().Get("saved") == "1"} + }{title, id, source, detail.Description, auth.GetCSRFTokenFromContext(r.Context()), r.URL.Query().Get("saved") == "1"} if err := h.renderer.Render(w, "task-detail-page.html", data); err != nil { http.Error(w, "Failed to render template", http.StatusInternalServerError) diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 8900f66..7b18cf1 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -2475,6 +2475,86 @@ func TestHandleCompleteAtom_DootShowsTitle(t *testing.T) { } } +func TestHandleCompleteAtom_Gtasks_LogsRealTitleAndInvalidatesCache(t *testing.T) { + h, cleanup := setupTestHandler(t) + defer cleanup() + + due := time.Now().Add(24 * time.Hour) + if err := h.store.SaveGoogleTasks([]models.GoogleTask{ + {ID: "g1", Title: "Renew passport", ListID: "list-a", DueDate: &due, UpdatedAt: time.Now()}, + }); err != nil { + t.Fatal(err) + } + if err := h.store.UpdateCacheMetadata(store.CacheKeyGoogleTasks, 60); err != nil { + t.Fatal(err) + } + h.googleTasksClient = &mockGoogleTasksClient{} + + req := httptest.NewRequest("POST", "/complete-atom", nil) + req.Form = map[string][]string{"id": {"g1"}, "source": {"gtasks"}, "listId": {"list-a"}} + w := httptest.NewRecorder() + h.HandleCompleteAtom(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", w.Code, w.Body.String()) + } + + completed, err := h.store.GetCompletedTasks(10) + if err != nil { + t.Fatal(err) + } + if len(completed) != 1 || completed[0].Title != "Renew passport" { + t.Errorf("completed log = %+v, want title %q (was hardcoded \"Google Task\" before the getAtomDetails fix)", completed, "Renew passport") + } + + meta, err := h.store.GetCacheMetadata(store.CacheKeyGoogleTasks) + if err != nil { + t.Fatal(err) + } + if meta != nil { + t.Error("expected gtasks cache metadata to be invalidated after completion, but it still exists") + } +} + +func TestHandleTaskDetailPage_GtasksSource_LoadsRealTaskFields(t *testing.T) { + h, cleanup := setupTestHandler(t) + defer cleanup() + + if err := h.store.SaveGoogleTasks([]models.GoogleTask{ + {ID: "g1", Title: "Renew passport", Notes: "bring photo", ListID: "list-a", UpdatedAt: time.Now()}, + }); err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest("GET", "/task?id=g1&source=gtasks", nil) + w := httptest.NewRecorder() + h.HandleTaskDetailPage(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", w.Code, w.Body.String()) + } + + mock := h.renderer.(*MockRenderer) + if len(mock.Calls) != 1 { + t.Fatalf("expected 1 render call, got %d", len(mock.Calls)) + } + type pageData struct { + Title string + Description string + } + jsonBytes, _ := json.Marshal(mock.Calls[0].Data) + var got pageData + if err := json.Unmarshal(jsonBytes, &got); err != nil { + t.Fatalf("failed to unmarshal render data: %v", err) + } + if got.Title != "Renew passport" { + t.Errorf("Title = %q, want %q", got.Title, "Renew passport") + } + if got.Description != "bring photo" { + t.Errorf("Description = %q, want %q", got.Description, "bring photo") + } +} + func TestHandleTimeline_IncludesBudgetStatusWhenTrackedTaskExists(t *testing.T) { h, cleanup := setupTestHandler(t) defer cleanup() |
