feat: item reminders — the fire-at-an-instant primitive, and one overdue rule for all four surfaces (IDEA-2641, closes #1010) - #1244
Merged
Conversation
…2641) Adds the storage, the scheduler tick, and the canonical event for one-shot item reminders (GitHub #1010). Nothing in Pad acted at a target time before this: a due_date makes an item show up as overdue once somebody asks the dashboard, so "revisit TASK-X on the 1st" had to live in an external cron. A TABLE, NOT A SCHEMA-FIELD ANNOTATION. The design sketch proposed marking schema date fields with a `reminds: true` key on models.FieldDef; recon overturned it. Such a key does not survive an ordinary collection edit, two independent ways: the web editor destructures each field into an EditableField and rebuilds a fresh definition key-by-key on save, so unknown keys are dropped (`pattern` and `unique_scope` survive only because two lines were hand-added for them), and models.CollectionSchema has fixed fields with no catch-all, so any Go unmarshal+marshal round-trip strips unknown properties — the hazard retargetRelationFieldsTx mutates raw JSON to avoid. Both failures are silent and both disarm a whole collection's reminders at once. It is the same defect class that moved traits out of the schema column in TASK-2657. The table also gives the lifecycle a home. A reminder is armed, then fired, then acknowledged, and a re-arm returns it to armed — per-reminder state a field definition has nowhere to keep. remind_at is an RFC3339 UTC instant, deliberately not a `date` schema value: those admit both YYYY-MM-DD and full RFC3339 and are compared against the SERVER'S LOCAL calendar day. A fire-at time cannot carry that ambiguity. The remaining timezone question for due_date is filed separately. Firing is one transaction per reminder carrying BOTH the fired_at write and the outbox insert. That pairing is the point: a fired_at committed without its event is a reminder that silently notifies nobody and can never be retried, because the row has left the armed set; an event without fired_at fires every tick forever. The UPDATE's own `fired_at IS NULL` predicate is the arbiter, so two instances ticking at once produce exactly one winner. item.reminder_due is admitted to the closed events/1 set as v1.2, with a new PayloadReminder family and no SSE name. The subject is the REMINDER, not the item: two reminders can be armed on one item, so an item-subject event could not say which fired, and the reminder id is what an acknowledgement addresses. A new payload family rather than reusing the item snapshot for the same reason — a snapshot would validate and still not answer the only question the event exists to answer. No SSE name in v1 because the poll surface is the contract; adding one later is additive, removing one is not. Ack is explicit and nothing else acks. An item reaching a terminal status deliberately does NOT ack: that would make every status write a reminder mutation, and it would silently consume a reminder set to fire after the work was done.
…four
Second half of IDEA-2641: the HTTP surface, the scheduler tick's wiring, and
the fix for the finding that justified the unit — `ready` / `next` did no date
handling at all.
OVERDUE NOW HAS ONE IMPLEMENTATION. It used to live inline in the dashboard's
attention loop, which meant `pad project stale` inherited it (it filters that
very list) and the recommendation surface never saw it. So a deadline reached
the two surfaces that REPORT on work and never the one an agent PULLS from.
overdue.go is now the only place that decides, and all four call it.
Two behaviour changes fall out, both deliberate:
- An overdue item bypasses the orphan branch's high/critical priority gate.
That gate was where a deadline quietly stopped: a low-priority item three
weeks late was reported by `stale` and never suggested by `next`.
- Overdue sorts above in-progress. The list is capped at three, so a rank
below in-progress would not merely order the deadline lower — on any
workspace with three things in flight it would keep an overdue item off
the surface entirely, which is indistinguishable from not shipping this.
The server-local-today comparison is UNCHANGED and known to be wrong for
multi-timezone deployments; it is filed as its own item with the cloud case
stated. Changing what "overdue" means on every existing instance inside a
change about where the rule LIVES is the kind of behaviour change nobody
reviews.
Fired reminders reach `next` / `ready` two ways, from one filtered list:
PendingReminders is the addressable form (it carries the id an ack needs), and
a prepended suggestion is the rendered form. They are prepended AFTER the cap
rather than entered as ranking candidates — a reminder is not a task competing
on priority, and whether it appeared should not depend on how busy the
workspace is.
Terminal-item reminders are FILTERED from the surface, never acked. Acking on
terminal status would couple every status write to reminder state and would
consume a reminder armed to fire after the work was done. The row stays
exactly as the user left it; the distinction is observable, and asserted.
Three guard tests caught this change and each was answered rather than
silenced:
- The request-body reader guard was right: the handlers now go through
decodeJSON, inheriting the NUL refusal and the size cap.
- The canonical-events guard was right: item.reminder_due is admitted to the
duplicated contract table as SPEC-3 v1.7, with the reminder subject kind
and the new payload family. SPEC-3's own text owes the same amendment.
- The NUL census asked for a decision on eight new columns. None carries
caller text: ids and FKs are server-generated, four are the server clock,
and remind_at is now re-parsed and re-formatted in the STORE as well as at
the edge — so the stored value is always machine-produced from a parsed
time and no caller bytes reach the column. The doc comment that used to
say "the caller normalizes" protected nothing.
Regenerating the baseline also found that GEN_NUL_BASELINE=1, which the
test's own instructions name, was never implemented — the flag did
nothing, so the documented path was hand-editing the file. Implemented, so
the next reader gets the mechanism the instructions promise.
Every test here was designed against a specific mutation and the mutation was RUN. A green suite proves nothing about a suite nobody tried to break, and three of the mutants I first wrote were not experiments at all. Store (10 mutants, all killed): candidate predicate <= flipped to >=; the event emission lifted out of the fire transaction; the fire UPDATE's `fired_at IS NULL` arbiter removed; the RowsAffected check ignored; re-arm clearing fired_at but not acked_at; ack losing `fired_at IS NOT NULL`; the poll surface losing `acked_at IS NULL`; normalizeRemindAt no longer refusing; it dropping .UTC(); GetReminder losing its workspace scope. Surfaces (12, all killed): the priority gate no longer bypassing on overdue; the sort no longer ranking overdue first; attention leaving the shared helper; the reason losing its OVERDUE prefix; the comparison flipped to >; terminal items no longer skipped; terminal reminders no longer filtered; the filter ACKING instead of hiding; reminders appended instead of prepended; the tick running on a far-future clock; ack answering 200 for an unfired reminder; parseRemindAt accepting a bare date. THREE MUTANTS DID NOT COUNT ON THE FIRST PASS and were rewritten. Two failed to compile (`if false` orphaned a variable; deleting a parse orphaned an import) and one had an anchor matching two call sites. A non-compiling mutant emits zero FAIL lines and reads exactly like a surviving one — it invents a hole that is not there — so the harness reports BUILD-FAIL and ANCHOR-BAD as outcomes distinct from SURVIVED. It also restores files from an in-memory copy rather than `git checkout`, which would delete uncommitted work in the tree. ONE MUTANT GENUINELY SURVIVED and the test was at fault, not the mutant: appending rather than prepending reminder suggestions was undetectable because the fixture had a single item, so the reminder sat at index 0 either way. The fixture now fills the three-item cap with in-progress work, where an appended reminder lands fourth and vanishes. Faithful mutant, weak test — checked in that order. The same lesson shapes the four-surface fixture: it is a LOW-priority open orphan, because that is the case the old code handled worst. A high-priority task would have made the ready/next leg pass against the unfixed tree, which is a green that measures nothing. Negative controls throughout: a future deadline is not overdue and does not reach the gate bypass; a tick with nothing due fires nothing; a completed item is neither overdue nor suggested. Without them a helper that reported every date, or a tick that fired everything, would satisfy every positive leg. The lead's pin is asserted in both directions: a fired reminder on a done item is ABSENT from the surface and PRESENT and still unacknowledged in the table. Asserting only the absence would pass against an implementation that consumed the row, which is the behaviour the pin exists to forbid.
An agent that can RECEIVE a reminder but not set one has half the primitive.
The poll surface is pad_project.next / ready, both long exposed, so reminders
already reached agents — what was missing is the other half: deferring a piece
of work is exactly the moment an agent knows when it wants to be asked again,
and it had no way to say so.
Two additive actions, two optional params. Nothing existing moved, so a v0.27
consumer enumerating neither is unaffected — the v0.13 / v0.11 / v0.8
disposition, which likewise wired existing CLI verbs onto the catalog.
remind_at REFUSES a bare date rather than reading it as midnight. Worth
stating because the `date` schema type accepts YYYY-MM-DD and a caller will
reasonably try it here: a bare date names a 24-hour span, and choosing an hour
inside it would fire at a time nobody picked.
Re-arm and disarm stay CLI-only. Both address a reminder by an id the agent
would have to list first, and no listing action exists on this surface — a
door with no handle. Adding them later is additive.
Five guards had to be taught, and each was answered on its merits rather than
excluded: the HTTP parity test (route mappers added, so the actions work on
the remote transport rather than being advertised and unrouted), the
read-only catalog's cmdhelp fixture and expected cmdPath map, the field-
conflict classifier (remind_at / reminder_id are NOT field writers — a
reminder is a row in its own table addressed by its own id, so listing them
as classified sources would have pointed detectFieldConflicts at something
that is not a field source), and the instructions.md / README action tables.
That machinery is why the version bump is safe to make now, and it earned its
keep on this change: every one of the five failed on the first build after the
catalog entry landed.
CONVE-23 sweep for prose this falsifies:
- SPEC-3 (DOC-2653) amended to v1.7 in the room, recording item.reminder_due
with its new subject kind and payload family — the first canonical event
with no user mutation behind it, since a scheduler tick produces it.
- CLAUDE.md gains the reminder routes, the CLI verbs, and the v0.28 entry.
It was also stale at 0.26 with NO v0.27 entry at all: the 0.27 unit swept
instructions.md and README.md and missed this file. Both added.
- skills/pad/SKILL.md gains the verbs and a routing entry, including the two
things an agent will get wrong — the time is an instant, so ask for a time
of day rather than picking one, and finishing the item does not
acknowledge the reminder.
Round 1 found four defects and refuted none of them. Each fix carries a test that fails against the code as it was, and each of those was mutation-checked. **P1 — pending reminders bypassed item-level visibility.** Every other dashboard section reads `allItems`, which the store already scoped to the caller's collections AND their granted item ids. The pending-reminder list is a direct workspace-wide query and inherited none of that, so a guest holding a grant on ONE item could read the refs and titles of every other item in the collection through its reminders — an item-level leak wearing a notification's clothes. Now filtered with the same `isItemVisibleToGuest` call the sibling sections use. The test's two items share a COLLECTION on purpose: a collection-level filter was already applied, so separate collections would have made it pass against the unfixed code. **P1 — soft-deleted items could starve the queue permanently.** Candidate selection ignored `deleted_at`, and `fireOneReminder` rolls back when it finds the item gone — which leaves the reminder ARMED and therefore a candidate again on the next pass. Candidates are ordered oldest-first and bounded by a limit, so enough archived reminders fill every batch and no live reminder ever fires. Silent, too: the tick reports zero fired and looks idle. Excluded in the candidate query rather than skipped downstream, so those rows never occupy a slot; the reminders themselves are kept, so restoring an item restores its reminder with it — asserted, because a fix that reaped them would pass the starvation test alone. **P2 — the pass stopped at the first failing reminder.** The per-reminder transaction exists precisely so one unfireable row cannot hold back the rest, and `return fired, err` made that comment false — with candidates oldest-first, one persistently broken old reminder blocks every newer one forever. Now continues and joins the errors, so a pass that fired seven and failed three reports both halves rather than reading as clean. The loop is split behind an injected seam because a real mid-transaction failure is not reachable from outside: the database refuses the corrupt rows that would cause one (verified — invalid JSON in items.fields is rejected by the schema). **P2 — suggestions dropped the reminder id.** The docs tell an agent to acknowledge what it sees in next/ready, and the payload carried no handle: a stateless poller could read the reminder and had no way to retire it, so it would be shown the same item forever. `DashboardSuggestion` now carries `reminder_id` (omitempty), `pad project next` prints the exact ack command, and the test acks with the id the surface handed out rather than merely checking the field is populated — a wrong-but-present id satisfies equality with itself. Four mutants, four killed; one was rewritten first because its anchor matched two call sites and was therefore not an experiment.
**`--rearm` was unusable.** `ExactArgs(1)` forced an item ref that the rearm branch then ignored, so the flag could not be reached without supplying a ref that was silently discarded. Now `MaximumNArgs(1)`, with each mode checked explicitly: a ref is required to arm, and a ref supplied ALONGSIDE `--rearm` is refused rather than ignored — it names an item the reminder may not even belong to, and quietly dropping it is how a user learns nothing about the reminder they just moved. **`unremind --format json` emitted plain text**, breaking the parseable-output contract every sibling command honours. **The MCP `ref` param did not list `remind`.** Agents read that flat description to decide what to send, so an action missing from it is an invalid call waiting to happen. It now also says what `ack-reminder` takes instead, and why: a reminder is addressed by its own id because an item can carry several. **Fractional seconds fired early.** `time.Parse` accepts `09:00:00.900Z` and `Format(RFC3339)` drops the fraction, so it was stored as `09:00:00Z` and fired 900ms BEFORE the moment the caller named — silently, having rewritten their value on the way in. Seconds are genuinely the stored resolution (the column is compared as a string against a whole-second clock, and the tick runs every 30s), so the only question was which way to resolve it, and truncation resolved it the wrong way. `NormalizeInstant` now rounds UP: at most a second of lateness, in exchange for a guarantee that can be stated — a reminder never fires before the instant it was set for. Late is a reminder; early is a wrong answer. Whole seconds round-trip exactly, which is asserted, because an implementation that added a second unconditionally would otherwise pass. Three mutants for this round, three killed (round-up→truncate, round-up→unconditional-add, MaximumNArgs→ExactArgs). Thirty across the unit. Two fixes carry no dedicated test and it is worth being explicit rather than implying coverage: the `--format json` branch on `unremind` is a one-line output change with no server-free way to drive it, and the MCP `ref` description is prose the drift tests do not read — they assert an action is DOCUMENTED, not that a param's sentence lists it.
…y on the arm response CONVE-23 follow-through on the round-1 fix. Both agent-facing docs told a caller to acknowledge a reminder with the id "returned when you armed it" — true, and useless to the caller that matters: a poller reading next/ready never armed anything. The suggestion now carries reminder_id and `pad project next` prints the exact ack command, so the docs say that instead. The prose was written before the fix existed, which is exactly the case CONVE-23 is about: a change that makes an instruction stale without touching the file the instruction lives in.
…ONVE-19) Every other test in this file calls runReminderTick directly. That vouches for the component and says nothing about whether anything ever calls it — a tick that is never started is indistinguishable, from those tests, from one that is. It is the convention's exact case, and the failure I recorded on my own identity doc three times in one unit: I test the component and not the binding. Driven through the injectable tick channel so the assertion pins a SPECIFIC pass instead of racing a 30-second ticker, and polled to a bounded deadline so a loop that never runs FAILS rather than hanging the suite. Mutant: drop `s.runReminderTick()` from the select and this goes red while every direct-call test stays green. Killed. The idempotence leg exists because a second Start spawning a second loop would leave one running after Stop, making the BUG-842 drain invariant false for this sweeper specifically — the one property a copied lifecycle is most likely to get right by accident and least likely to be checked. The cmd/pad call site (cmd_server.go, alongside StartTokenReaper) stays verified by inspection: a source-scanning guard for it would be an instrument asserting facts about source, which is code with an adversary and not worth it for one line that sits in the middle of five identical neighbours.
… the poll surface was unbounded **A re-arm mid-pass did not stop the fire.** The candidate scan selects an id; before the UPDATE runs, a `--rearm` can move that reminder into the future. Re-arm clears `fired_at`, so a predicate checking only `fired_at IS NULL` still matched — the pass fired a reminder the user had just deferred and emitted its event. The re-arm cannot undo that: it can clear the mark, but the event is already on the outbox and at-least-once means a consumer has seen it. The fire UPDATE now revalidates `remind_at <= nowTS` against the SAME nowTS the candidate scan used. Same-value deliberately: the arbiter and the scan must agree about when this pass is, or a reminder could pass one and fail the other for no reason but clock drift inside a single pass. **The poll surface was unbounded.** Every fired-and-unacknowledged reminder was loaded and turned into a suggestion prepended to a list that is otherwise capped at three, so a workspace with five hundred unacknowledged reminders returned five hundred suggestions — in the dashboard response, the hottest read in the product, growing until somebody acknowledged them. Two bounds, because they are two different guarantees: the query takes a window (default 50, oldest-fired first, so it holds what has waited longest), and the prepended suggestions are capped at 5 so `suggested_next` stays a recommendation rather than a second inbox. The full set stays addressable in `pending_reminders`. Truncation is REPORTED as a boolean, not a count. A count would have to be post-visibility-filter to be true for the caller reading it, and the store cannot compute that — the filter runs per item, above. "There are more than you can see here" is the strongest claim the data supports, so it is the one made. Four mutants; two killed outright, two survived and were run down under CONVE-28: - **Uncapped suggestions survived because the fixture had ONE reminder** — capped and uncapped are the same list at n=1. That is the SECOND time a single-item fixture hid a count-or-order property in this file. Fixture now arms eight; it also asserts all eight remain in `pending_reminders`, so the cap is pinned to the recommendation and not to the data. - **Removing the SQL LIMIT survived, correctly, and the test comment now says so.** The Go slice cap bounds the PAYLOAD; the SQL LIMIT bounds the DATABASE'S work. Only the first is observable at this level — with the LIMIT gone the response is still bounded, while the query silently goes back to materialising every pending row before discarding most of them. That is a memory and I/O property with no assertion available here, so it is stated as a coverage boundary rather than papered over with a green that would not have measured it.
…t one CONVE-23 inside the file the round-3 fix touched. The comment described the UPDATE as an arbiter for concurrent TICKS, which is what it was written for and is why I did not re-read it when asked whether a user edit could race the pass. It now says what it actually defends against, and names the general shape: an arbiter is only an arbiter with respect to the writers it can see.
…-1 starvation Round 3 bounded the poll surface. Round 4 caught what that bound did: the query took the first N rows and the dashboard then discarded the ones it could not show — hidden items, unauthorised items, completed items — so N such rows hide a visible reminder behind them indefinitely, with no continuation to reach it. That is the SAME defect I had removed from the fire path one round earlier, reintroduced in the read path within the hour. The general form is worth stating because I clearly did not hold it: **a bounded window is only safe when the discarding happens BEFORE the bound.** Filtering above a limit is a starvation every time, and it does not matter what the filter is for. Two halves, because the two filters are not the same kind of thing: **Visibility is now scoped IN SQL**, using the same collection-id / item-id sets every other dashboard section gets through `allItems` — the same three-way shape as ItemListParams, where holding both collection grants and item grants is an OR. Invisible rows no longer occupy the window at all, which is strictly better than filtering them out afterwards and is what the sibling sections have always done. **Terminality is paged**, because SQL cannot evaluate it — a collection's schema defines which statuses are terminal. The collector refills from the next page when a page comes back short, bounded by a max scan so a workspace full of completed items cannot turn a dashboard read into a table scan. The bound is 10x the window: the common shape fills on the first page, and the pathological shape terminates in a fixed number of indexed reads. Stopping at the scan bound reports truncation, which is honest — there may be more, and we did not look. The empty-scope case is a THIRD state that reads like the second: nil CollectionIDs means unrestricted, a non-nil EMPTY slice means this caller sees no collections. Without an explicit guard they collapse, because the switch matches none of its cases at length zero and adds no clause at all — so "nothing visible" would return the whole workspace. Three mutants, one survived: the empty-scope guard, because no dashboard-level test produces that state (callers that would are refused earlier by workspace access). Faithful mutant, missing test — it now has a direct one, with a sanity leg so a build returning nothing cannot pass it by accident. A guard for a state nothing exercises is exactly the one that rots.
… over stdio
**P1: local stdio MCP `remind` was unusable.** cmdhelp derives positionals by
regex from a command's `Use` string, and `<instant>` inside
`remind <ref> --remind-at <instant>` matched — it became a second REQUIRED
positional, so dispatch failed with `missing required argument "instant"`. The
action was advertised on a transport where it could not run.
**The MCP catalog's own tests did not catch it, and the reason is the finding.**
That suite builds its cmdhelp document BY HAND: I wrote `Args: mkArgs("ref")`
in it, so the fixture agreed with what I meant rather than with what the CLI
says. Five parity and drift tests passed against a document I authored to
match my own intention — the "a test that agrees with whatever the table says
is not a test of the table" shape, which the canonical-events test warns about
in its own comment two packages away. The new test reads the REAL command tree
via cmdhelp.Build, which is the only thing in this repo that can disagree with
me about what the CLI declares.
**P2: `pad project ready` withheld the ack handle** that `next` prints.
Showing a fired reminder on the surface an agent polls while withholding the
id it needs to retire it means the same entry comes back on every poll,
forever.
**P2: suggestions asserted a collection they did not have.** The orphan branch
admits ANY collection — its own comment claimed it gated on tasks "mirroring
the active-plan branch", and that comment was simply false — while the output
hardcoded `Collection: "tasks"` and the reason said "Open task". Pre-existing
for high-priority items since BUG-1082; my overdue bypass widened it to any
overdue item, which is how it surfaced.
Fixed by carrying the item's REAL collection rather than by narrowing the
branch: narrowing would silently drop the non-task items this has surfaced for
a year, and the defect is the mislabelling, not the inclusion. The false
comment is replaced with what the code actually does.
The first version of that test used an overdue IDEA and SKIPPED — ideas use
`new`, and the branch requires `open` or an active status, so it never became
a candidate. A test that cannot fire is a failed reconstruction, not a pass;
the fixture is now a bug-like collection whose vocabulary contains `open`,
which is the population the defect can actually reach.
Three mutants, three killed. Forty-one across the unit.
…kspaces **P1, and the only defect in this unit whose consequence leaves the process.** Workspace soft-delete deliberately keeps items for the 30-day restore window, so the candidate query's filter on the ITEM's deleted_at found nothing wrong — and the tick kept firing, emitting outbound webhook events for a workspace whose owner had deleted it, possibly while deleting their account. Both queries now join workspaces and require `w.deleted_at IS NULL`. Nothing is destroyed: a restored workspace resumes firing, which the test asserts, because "stops firing" and "is destroyed" are very different answers to someone who restores a workspace and only one of them is right. That test first failed for the WRONG REASON and the fixture was at fault: it counted every outbox row in the workspace, and item creation writes its own, so the assertion was satisfiable by the fixture itself and discriminated nothing. Scoped to the reminder event type. **`Use: "remind <ref>"` declared a requirement the command contradicts.** cmdhelp derives the machine-readable arg spec from that string, and `--rearm` takes no ref — so the published contract said "required" for something optional. The requirement is CONDITIONAL, which cmdhelp cannot express, so the honest declaration is `[ref]` plus the explicit check that names both call shapes. The round-5 test grew a `required` column, which is what makes this observable at all: asserting only the arg NAMES would have passed. **The pad_item tool description omitted both new actions.** The params were declared and the actions dispatched, but the prose an agent reads to decide what a tool can do did not mention them — discoverable only by someone who already knew to look. It now describes both, including the two things an agent gets wrong: remind_at is an instant, and nothing but an explicit ack retires a fired reminder. Three mutants, three killed. Forty-four across the unit.
…biter Third instance of one class, so this fixes the SHAPE rather than the instance. The class: the candidate scan filters on something the fire transaction does not revalidate, so a change committed between them fires a reminder that no longer qualifies. Round 3 was a re-armed instant. Round 1's soft-deleted item was the same thing caught from the other side. Round 7 is a workspace deleted between the scan and the fire — the round-6 fix added the condition to the SCAN only, and the arbiter went on not knowing about it. Fixing those one at a time is what let the third happen. `reminderFireable` is now a single string that both sites reference: the scan asks it and the fire UPDATE re-asks it, so they cannot disagree, and a fourth condition is one edit in one place rather than two edits someone has to remember are paired. Written as a correlated EXISTS on item_reminders.item_id rather than a JOIN precisely so the identical text is valid in both a SELECT and an UPDATE, and the scan drops its table alias so the two uses are the same characters. What deliberately stays outside it: `fired_at IS NULL` and `remind_at <= ?` live on the reminder row itself, are already spelled identically at both sites, and folding them in would need a parameter order the shared form cannot express. Said in the comment so the omission reads as a decision. Both directions are now tested at the arbiter — a workspace deleted mid-pass and an item deleted mid-pass — because the item case previously relied on the item load coming back nil, and someone simplifying the EXISTS down to the workspace check alone would otherwise still see green. Three mutants, three killed: the arbiter dropping the shared predicate, and the predicate dropping each of its two halves. Forty-seven across the unit.
…ry reminder WorkspaceExport is a hand-maintained field list, so a new table joins it only if someone remembers. Reminders did not: a backup/restore, or a SQLite→Postgres migration via `pad db migrate-to-pg`, dropped every pending reminder with nothing in the destination to show anything had gone. The line that list has always drawn is item-scoped workspace CONTENT (comments, links, versions — exported) versus per-user state (stars, watches — not). A reminder has no user column and hangs off an item, which puts it on the exported side. Stating the rule rather than just adding the field, because the next person adding a table needs to know which side they are on. LIFECYCLE MARKS ARE CARRIED, not reset. A fired-and-unacknowledged reminder is still owed to whoever armed it, so it arrives pending; an armed one whose instant has passed fires once on the destination's first tick, which is what would have happened had the workspace never moved. Re-arming everything on import would invent a schedule the user did not set. NULL rather than empty string for the unset marks — the lifecycle is defined by NULL-ness, and "" would make a never-fired reminder read as fired at "". TestMigratedTablesCoversTheExport caught the second half, which I would have missed: `pad db migrate-to-pg`'s NUL preflight decides what to REFUSE on from MigratedTables, so a table the migration copies and the preflight does not know about is a gap in exactly the guard that exists to prevent one. Added there too, with the reason it can never actually fire — every column is machine-produced, so it is listed for coverage rather than expectation — and the "six tables" prose it falsified is now seven. Two mutants, two killed: export dropping the block, and import discarding the marks. Forty-nine across the unit.
…variant The lead's read on why rounds 4 and 7 were the same class: the fire path had no stated invariant, so each fix defended an instance. This states it, and derives the pin from the paragraph rather than from the bug history. THE INVARIANT: the candidate scan is a hint and may be assumed to prove nothing. Every condition that made a row a candidate is re-asserted inside the transaction that marks it fired, in the same statement that does the marking, so checking and writing are one atomic act. Worded as "the scan proves nothing" rather than as a list on purpose — a list invites the next person to add a condition to the scan and stop, which is exactly what happened four times here. TestFirePathInvariant is the pin: one table, one row per scan-side condition, each invalidating that condition in the window between the scan and the fire and asserting the same three things — nothing fires, no event leaves, the reminder is not consumed. The earlier per-defect tests are folded in as rows; they said the same thing one instance at a time, which is how four of these shipped. Adding a fifth condition to the scan without a row here should feel like an omission. It carries a positive control, because four cases that all assert nothing happens would pass against a build that never fires at all. The matrix immediately falsified a claim in the paragraph I had just written. I wrote that the item load inside the transaction is "for the payload, not for the check"; removing the item half of reminderFireable alone changes no observable behaviour, because the load then returns nil and the deferred rollback undoes the write. Item liveness is defended TWICE and a single-mutant experiment cannot say which guard is carrying it — removing both is what kills the test. Both are kept, the predicate is named as primary (the row never matches, so no write happens at all), and the asymmetry is stated: workspace liveness has no second line, which is why dropping ITS half does fail the pin. Six mutants: five singles plus the pair. Five killed alone; the item single survives by design and is documented as such rather than left as an unexplained green. Fifty-five across the unit.
**P1: items.item_number is NULLABLE and I scanned it into an int.** Migration 006 added the column to existing rows, so a pre-numbering item still carries NULL — and scanning NULL into an int fails the Scan, which fails the QUERY, which degrades the whole pending-reminder section. One old row, and the feature is dark for everyone in that workspace. ListWatchesForUser, which this query was modelled on, uses sql.NullInt64 for exactly this column. I copied its shape and dropped the part that handles the column's actual nullability — the same way of being wrong as the round-5 cmdhelp fixture: borrowing a form without borrowing what it knows. The legacy row now carries no ref rather than a fabricated "PREFIX-0", which would name a different item. **P1: export shipped reminders that import could only discard.** The items section filters on deleted_at IS NULL, so a soft-deleted item is not in the bundle and its reminder can never be reunited with it. My comment claimed the item_links rationale — round-trip the raw graph so a restore reunites them — which is true for links and false here, because links keep soft-deleted endpoints in the bundle and items do not. A link is a row ABOUT two items; a reminder whose item is absent is a dangling schedule. **P2: import wrote remind_at raw.** Import is a writer, and a bundle is not necessarily one this server produced — hand-edited, or from another instance. A local offset or a bare date would land in the one column every comparison downstream treats as a UTC instant, firing early, late, or never. It now normalizes like every other door. An unparseable value is SKIPPED with a warning rather than failing the restore, matching the lenient import-side precedent already in this file, and the raw value's LENGTH is logged rather than its content. Three mutants, three killed; two needed rewriting because the single-line form did not compile — reverting the nullable scan also requires reverting the render, and dropping the normalization orphans a variable. PROCESS FAULT, recorded because it makes this round's findings weaker than they look: I edited the tree while this review was reading it — committed the invariant work and ran five mutation experiments, which write and restore source, over the same files. A review binds to the tree it read and I moved it underneath. Every finding above was re-verified against the current tree before being acted on, and the next round runs with no concurrent edits.
…store An ORPHANED item — one whose collection is missing from the bundle — still gets an itemMap entry. It has to: the entry is written before the skip because parent resolution inside the same loop reads the map for items it has not reached yet. So `itemMap[x] != ""` is satisfied by an id that names no row, and inserting a foreign key to it fails (SQLite enforces FKs here via the DSN's `_pragma=foreign_keys(on)`; Postgres always does). The pre-existing mapping is the sharp edge. The aggravating half was mine: this loop treated a failed reminder insert as FATAL, where item_links and item_versions both skip, so one orphaned item carrying a reminder rolled back an entire 900-item workspace restore. A reminder is the least critical thing in a bundle and it had the strictest failure handling in the file. Both halves fixed: the loop gates on items that actually landed, and a failed insert warns and skips like its siblings. TWO GUARDS THAT ONLY DIE TOGETHER, and this is measured rather than assumed. Reverting either alone leaves the test green — with the map gate restored the skip survives the FK failure, and with the fatal return restored the gate means the insert never fails. Removing both is what fails it. They are kept as a pair because they defend the same failure at different depths (prevent the bad write / survive a bad write arriving some other way), and the pair is recorded in the code so a future reader does not delete one as dead after watching its mutant survive. Second time this shape appeared today; the first was item liveness on the fire path. The bundle in the test is hand-built, because ExportWorkspace cannot produce an orphan — which is the reason it needed a test. That shape only arrives from a hand-edited or foreign bundle, and surviving those is what import is for. Three mutants: two singles that survive by design, plus the pair that kills. Sixty-one across the unit.
…ther unit's **suggested_next returned up to eight entries against a cap of three.** Round 3 prepended reminders PAST the list's own cap, reasoning they should not compete for slots. Every consumer — the web dashboard, `pad project next`, `pad project ready` — is written for three. Worse, it silently falsified a decision recorded elsewhere: BootstrapDashboard deliberately has no suggested_next_overflow_count BECAUSE this list is capped at three upstream, and its comment names raising that cap as the moment to add one. My change made another unit's reasoning wrong in a file I never opened. The combined list is now trimmed back to three, reminders still leading — a reminder can push a task suggestion out, which is the right way round, and the full set stays addressable in pending_reminders. My first version of that trim used `limit`, which is REASSIGNED above to len(candidates) — so on a workspace whose only entries are reminders it would have truncated to zero, killing precisely the case the surface exists for. Caught by reading the surrounding lines before running anything; it has its own test now. **pending_reminders was uncapped in the bootstrap projection.** BootstrapDashboard embeds *DashboardResponse, so every new field joins the boot payload automatically — here, a window of up to 50, which is the budget PLAN-1410 spent a unit trimming. Capped at 5 with an overflow count, under its own constant rather than borrowing bootstrapAttentionCap: they answer different questions and a future change to one must not silently move the other. **Truncation was reported from the wrong question.** The collector used the store's `more` flag, which answers "is there another PAGE", not "did I read all of THIS one" — so a window filling part way through the final page reported that the caller had seen everything while unread rows sat behind the fill point. The paging bounds are now injectable so the case is testable at all: building it with a window of 50 needs ~75 rows in a specific pattern, with a window of 3 it is four. **Import accepted acked-without-fired**, which is not one of the lifecycle's three states. Such a row fires, is excluded from the pending surface because it is already acked, and can never be acknowledged because AckReminder requires acked_at IS NULL — an event emitted into permanent invisibility. The acknowledgement is dropped and the schedule kept, since an ack of something that never fired means nothing. Five mutants, five killed (one rewritten — removing the flag orphans a variable). Sixty-six across the unit.
… ack from the ack Four P2s from round 12 (two independent runs, both landing on the same line of the fire path), each closed at the layer where it lives: - fireOneReminder pins the item and workspace rows FOR NO KEY UPDATE on Postgres before the arbiter UPDATE. reminderFireable re-asserted liveness at the predicate's instant and nothing held it to the commit instant; under READ COMMITTED an archival could commit in between and the event left the process about a deleted resource. Same idiom and same lock strength as CreateAttachmentForLiveItem; SQLite is excluded by its BEGIN IMMEDIATE, not skipped for convenience. Two PG-only pins verify "blocked" in pg_stat_activity, not by elapsed time; the pin-removed mutant fails both. - CreateReminder asserts "live item of THIS workspace" in the INSERT's own SELECT and returns ErrReminderItemGone otherwise. The table had an FK and no same-workspace constraint; a mismatched pair fed another workspace's title to this one's dashboard and webhooks. Handler maps it to 404. - AckReminder matches every fired row (COALESCE keeps the first ack, updated_at moves only when acked_at does), so a no-match means exactly "not fired at the instant of the ack". The handler no longer decides 409-vs-200 from the row it read before the UPDATE. - The invariant paragraph gains its missing sentence: "at that instant" means the commit instant, and the pin is what makes the predicate's instant and the commit instant the same one. Round-12 caveat carried: both runs were static reads (sandbox blocked Go's build cache), so "four" is a floor, not a measurement. Refs IDEA-2641
…th its item's, at every read Every reader scoped by r.workspace_id and then joined the item without asserting the two agree. No door writes a disagreeing row today (CreateReminder derives the pair from the item; import maps within the workspace), and the table has nothing that forbids one — so a hand-edited bundle, a future move door, or a direct write would carry one workspace's item into another's dashboard, export, and webhooks. The identity goes into reminderFireable (scan + arbiter), the Postgres row pin, ListPendingReminders and the export query. One test writes the row raw — the only way one can exist — and asserts it is inert at each site; the predicate-removed mutant scans and fires it. Refs IDEA-2641
…he same identity as every other read GetReminder scoped by the row's own workspace_id and ListRemindersForItem by item_id alone, so a row whose two columns disagree — the class rounds 12 and 13 closed at the scan, the arbiter, the pin, the pending surface and the export — was still readable through the two reads that reach a single row. reminderOwned is that identity on its own, without the liveness half those two reads must not have (a fired reminder on an archived item is history worth showing). The write paths reach a row only through GetReminder, so scoping it scopes them; a row no door can write needs no door to delete it. ListRemindersForItem now takes the workspace its caller already resolved the item in. The raw-row test asserts both reads refuse the row from both sides; the reminderOwned-removed mutant surfaces it through GetReminder. Refs IDEA-2641
…dable, and its verbs say "archived" The doors resolved the item live. Listing an archived item's reminders answered 409 from a GET, and ack/re-arm/delete answered a bare 404 for a reminder that exists on an item that exists — while the store, since round 14, deliberately keeps that history readable. The API already has a posture for archived items: GET reads them, mutations answer 409 "archived … restore it before editing" (writeItemResolveError). The list now follows handleGetItem; the lifecycle verbs load the item include-deleted, run the visibility check first, and then answer the same 409 every other item mutation does. One test walks archive → list 200 / ack 409 / arm 409 → restore → ack 200 on the same rows. Refs IDEA-2641
…d 409 by slug, and the door courtesy named Three findings on the server pass. (1) An item that was both a fired reminder and an ordinary candidate appeared in suggested_next twice; the ordinary entry is dropped, the reminder entry (which carries the ack id) stays, and two reminders on one item remain two entries. (2) Round 16's 409 for an archived item's reminder was written by re-resolving item.Ref, which is derived and empty for a legacy item with no item_number — so the class most likely to be legacy fell through to a bare 404. The slug is handed over instead. (3) The archived check in resolveReminderForWrite is check-then-write, and an archive landing in between lets the verb through: accepted and documented — it is the posture of every item mutation here (UpdateItem's UPDATE has no liveness clause), the outcome is benign, and putting liveness in AckReminder's WHERE would re-create the no-match ambiguity round 12 removed. Refs IDEA-2641
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1010. Implements IDEA-2641.
Pad's date handling was entirely reactive: a
due_datemakes an item show up as overdue once somebody asks the dashboard, but nothing ever acted at a target time — so "revisit TASK-X on the 1st" had to live in an external cron. This adds the engine, and fixes the finding that justified the unit.Two behaviour changes, called out because they are not in the feature's name
1. An overdue item now bypasses the suggestion gate. The orphan branch only surfaced open items at
high/criticalpriority. That gate was where a deadline quietly stopped: a low-priority item three weeks late was reported bypad project staleand never suggested bypad project next.2. Overdue now sorts above in-progress. This is not cosmetic.
suggested_nextis capped at three, so ranking overdue below in-progress does not merely order it lower — on any workspace with three things in flight it keeps the overdue item off the surface entirely, which is indistinguishable from not shipping the change.Both follow from the recon finding:
ready/nextdid no date handling at all. The complaint in #1010 is sharper than "not honored uniformly" — overdue reached the two surfaces that report on work and never the one an agent pulls from.Deliberately NOT changed: the comparison is still lexicographic against the server's local calendar day. That is wrong for multi-timezone deployments and is filed as its own item with the cloud case stated. Changing what "overdue" means on every existing instance, inside a change about where the rule lives, is the kind of behaviour change nobody reviews.
The reminder primitive
A
remind_atinstant on an item, a scheduler tick, anditem.reminder_dueon the existing outbox → webhook rails.It is a table, not a schema-field annotation — the design sketch proposed marking date fields with a
reminds: truekey onFieldDef, and recon overturned it. Such a key does not survive an ordinary collection edit, two independent ways: the web editor rebuilds each field key-by-key from an allowlist (patternandunique_scopesurvive only because two lines were hand-added for them), andCollectionSchemahas fixed fields with no catch-all, so any Go unmarshal+marshal strips it — the hazardretargetRelationFieldsTxmutates raw JSON to avoid. Both failures are silent and both disarm a whole collection's reminders. Same defect class that moved traits out of the schema column in TASK-2657.Firing is one transaction per reminder carrying both the
fired_atwrite and the outbox insert. Afired_atcommitted without its event is a reminder that silently notifies nobody and can never be retried, because the row has left the armed set; an event withoutfired_atfires every tick forever. The UPDATE's ownfired_at IS NULLpredicate is the arbiter, so two instances ticking at once produce exactly one winner.Ack is explicit and nothing else acks. Completing the item deliberately does not: that would couple every status write to reminder state, and would consume a reminder armed precisely to fire after the work was done. A reminder on a completed item is filtered from the poll surface and left untouched in the table — absent from
next, present and still unacknowledged in the row.The poll surface is mandatory, not a convenience.
deliverOutboxUnitacks an event immediately when no webhook dispatcher is configured — the common self-hosted shape — so a webhook-only reminder would be a no-op on most installs.Contract changes
item.reminder_dueadmitted to the closedevents/1set, with a newremindersubject kind andreminderpayload family, and no SSE name. The subject is the reminder, not the item: two reminders can be armed on one item, and the reminder id is what an acknowledgement addresses. It is the first canonical event with no user mutation behind it.pad_item.remind/ack-reminder. Agents already received reminders (the poll surface ispad_project.next/ready); what was missing is the half where an agent deferring work can say when it wants to be asked again.The fire-path invariant, and what "at that instant" means
Stated once on
FireDueRemindersand pinned byTestFirePathInvariant: the candidate scan is a hint and may be assumed to prove nothing. Every condition that made a row a candidate is re-asserted in the UPDATE that marks it fired —fired_at IS NULL,remind_at <= nowTS, item live, workspace live, and (round 13) the row's workspace is its item's — through one shared predicate,reminderFireable, so the scan and the arbiter cannot spell it differently.Round 12 found the sentence the paragraph was missing: a read is not a hold. The predicate re-asserted liveness at the instant it was evaluated and nothing kept it true until commit. On SQLite that gap does not exist (
_txlock=immediatemakes every transaction aBEGIN IMMEDIATE, so an archival cannot open while a fire is in flight). On Postgres under READ COMMITTED the UPDATE locks only the reminder row, soDeleteItem/DeleteWorkspacecould commitdeleted_atbetween the predicate and the outbox write, and anitem.reminder_duewebhook left the process about a deleted resource.fireOneRemindernow pins the item and workspace rowsFOR NO KEY UPDATEas its first statement on Postgres (the idiom and lock strengthCreateAttachmentForLiveItemalready uses): the archival waits for the fire to commit, or, having committed first, makes the pin's re-read miss and the fire returns without emitting. Two Postgres-only tests hold an uncommitted archival and verify "blocked" inpg_stat_activity, not by elapsed time; the pin-removed mutant fails both.Identity, asserted everywhere rather than trusted from the INSERT.
item_remindershas an FK toitemsand nothing tying itsworkspace_idto the item's.CreateReminderderives the pair from the item in the INSERT's ownSELECT(round 12; a cross-workspace or archived item isErrReminderItemGone, 404 at the door), and every read — the scan, the arbiter, the pin,ListPendingReminders, the export,GetReminder,ListRemindersForItem— assertsi.workspace_id = r.workspace_id(rounds 13–14). A row written raw with the two disagreeing is inert at all seven sites, by test. Making that a composite FK instead is filed as IDEA-2883.Ack answers from the ack, not a pre-read.
AckRemindermatches every fired row (COALESCEkeeps the first acknowledgement;updated_atmoves only whenacked_atdoes), so a no-match means exactly "not fired at the instant of the ack" and the handler never decides 409-vs-200 from the row it read before the UPDATE (round 12).Testing
Full Go suite green (gofmt +
go vetclean); Postgres differential run on a private container.Eighteen codex rounds; the store pass converged at round 15 and the server pass at round 18. Rounds 1–4 whole-diff, 5–11 whole-diff (Rook, day 12: MCP over stdio, soft-deleted workspaces firing, one predicate for scan and arbiter, export dropping every reminder, a legacy NULL
item_numberhiding every reminder, one orphaned item aborting a restore, four contract slips), 12–18 by package (claude, day 56). Store, rounds 12–15: the fire-path hold, item∈workspace on arm, identity at every read, ack from the ack; round 15's only finding was pre-existing export behaviour, filed as BUG-2884. Server, rounds 16–18: an archived item's reminders are readable and its verbs answer 409 "archived" like every other item mutation (16); one suggested_next entry per item when it is both a fired reminder and an ordinary candidate, the archived 409 by slug so a legacy item with noitem_numberdoes not fall through to 404, and the check-then-write archived courtesy accepted and documented as the API's posture (17); clean on the two server files (18). Rounds 1–4 in detail: Round 1: pending reminders bypassed item-level visibility (every other dashboard section reads a list the store already scoped; this one was a direct workspace-wide query, so a guest with a grant on one item could read every other item's refs and titles through its reminders); soft-deleted items could starve the queue permanently (a rolled-back fire leaves the reminder armed, so oldest-first archived rows refill the bounded batch forever while the tick reports zero fired and looks idle); the pass stopped at the first failing reminder, contradicting the comment two lines above it; and suggestions dropped the reminder id, so a poller could see a reminder and had no handle to retire it. Round 3: a reminder deferred mid-pass fired anyway (the candidate scan selects an id, a--rearmbefore the UPDATE moves it to the future, and because re-arm clearsfired_atthe predicate still matched — and the re-arm cannot undo it, the event is already on the outbox), and the poll surface was unbounded. Round 4: the round-3 bound recreated the round-1 starvation in the read path — the query took the first N rows and the dashboard discarded the ones it could not show, so N invisible or completed rows hide a visible reminder indefinitely. Visibility is now scoped in SQL and terminality is paged, because a bounded window is only safe when the discarding happens before the bound. Round 2:--rearmwas unusable,unremind --format jsonemitted text, the MCPrefdescription omittedremind, and fractional seconds fired early —09:00:00.900Zstored as09:00:00Z. Seconds are genuinely the stored resolution, so the fix is to round up: at most a second late, in exchange for a guarantee that can be stated. Late is a reminder; early is a wrong answer.55 designed mutants across the unit; the item-liveness single survives by design and is documented (see the invariant paragraph in code). Of the first 38: Three did not count as experiments on the first pass and were rewritten: two failed to compile (a non-compiling mutant emits zero FAIL lines and reads exactly like a surviving one) and one had an anchor matching two call sites. Four were rewritten before counting across the unit. Four survived at first and each was run down under "faithful mutant or weak test, in that order" — two were weak fixtures (both n=1, hiding a count/order property), one was a missing test for a guard no higher-level caller exercises, and one survived correctly: removing the SQL
LIMITleaves the payload bounded by the Go cap while the query goes back to materialising every row, which is a memory/IO property with no assertion available at that level. The test comment states that coverage boundary rather than claiming a green that would not have measured it. Earlier, one mutant genuinely survived — appending rather than prepending reminder suggestions — and the test was at fault, not the mutant: the fixture had a single item, so the reminder sat at index 0 either way. The fixture now fills the three-item cap.The four-surface fixture is a low-priority open orphan on purpose: that is the case the old code handled worst, so a high-priority task would have made the
ready/nextleg pass against the unfixed tree — a green that measures nothing.Guards that caught this, each answered rather than silenced
The request-body reader guard (handlers now go through
decodeJSON), the canonical-events guard, the NUL column census, the HTTP transport parity test, the read-only catalog's cmdhelp fixture, the field-conflict classifier, and the instructions.md / README drift tests. Every one failed on the first build after the relevant change landed.Two instrument defects found along the way and fixed here:
GEN_NUL_BASELINE=1was never implemented. The census test's own failure message tells you to regenerate with it and nothing read the variable — so the documented path was hand-editing the baseline, the one form of regeneration that can silently drop an entry.instructions.mdandREADME.mdand missed it. Both versions recorded.For the reporter
@thomasthinks offered to test on #1010. The shape is
pad item remind TASK-5 --remind-at 2026-08-01T09:00:00Z, thenpad project nextonce it fires, thenpad item ack <id>. A bare date is refused on purpose — it names a 24-hour span, and picking an hour inside it would fire at a time you did not choose. Recurrence is explicitly out of scope for v1.