fix(tasks): preserve task metadata on completion - #238
Merged
pufit merged 1 commit intoAug 1, 2026
Conversation
fallintoplace
force-pushed
the
fix/task-done-preserve-metadata
branch
from
July 30, 2026 19:45
5acb99f to
d351fce
Compare
pufit
approved these changes
Aug 1, 2026
pufit
pushed a commit
that referenced
this pull request
Aug 4, 2026
TaskManager.reindex() passes eight of upsert_task()'s nine parameters, but that
method replaces the whole row: its ON CONFLICT(id) DO UPDATE SET assigns status,
source, source_url, deadline and tags from excluded.* unconditionally. Every
argument the call omits or mis-keys is therefore written back as its signature
default, so a single reindex() corrupts four columns at once.
tags is omitted entirely, so the column that every tag reader filters on is
emptied. list_tasks(tag=), count_tasks(tag=) and all three search_tasks tag
paths share the predicate ',' || tags || ',' LIKE '%,<tag>,%', so the row stops
matching any tag, and the FTS mirror loses its tag tokens with it.
status falls back to the directory default, because fields.get("status", status)
reads a **Status:** frontmatter line that no writer emits. Unlike tags this is
unrecoverable: the value exists only in the database, so there is nothing on disk
to restore it from. On the workspace this was developed against, 392 of 395
active rows would reset to pending, which is the whole routing state. Reading
that field is wrong in any case. Twenty task files do contain such a line; most
hold prose, which was stored verbatim as the status, and four hold a value that
is exactly a configured status, so the read can also store a plausible but stale
one. The read is therefore removed rather than repaired.
source receives the frontmatter value from the **Source:** line, which holds the
source URL (handlers/tasks.py writes source_url there), while source itself is a
short vocabulary column. deadline and source_url are nulled whenever the file
omits their line.
parse_task_frontmatter is fixed in the same change, because the values reindex
now trusts have to mean what they say. Its value pattern was \s*(.+), where \s
matches newlines and (.+) demands a character, so a field written with no value
did not parse as empty: it consumed the blank line and captured the next
non-empty one. On this workspace, 84 of 1309 task files carry a tags value that
this changes, and a reindex would have stored titles, **Issue:** URLs and
markdown headings as a task's tags. The shape is not hypothetical:
task_update(tags="-lasttag") rewrites the line as **Tags:** with nothing after
it, mid-file. The pattern becomes [ \t]*(.*), which cannot cross a newline and
permits an empty value, so the key is still registered and the field reads as the
explicit clear it is. The value is optional rather than merely line-bounded
because 71 blank fields on disk have no trailing space either. This parser has
three production callers, so the change was measured against the whole suite
rather than argued: see the suite comparison below.
The fix supplies the values at the call site, using the idioms the six correct
upsert_task() callers already use, and leaves upsert_task()'s replacement
semantics untouched. That is the same shape and the same deliberate scoping as
PR #238, which fixed this class on the two completion paths and left reindex() as
the last caller treating a full-row replace as a partial update.
status now comes from the stored row, with the directory authoritative for
terminality: a file in done/ is terminal by definition, and a stored done on an
active/ file is the orphan state docs/tasks.md forbids, so it is corrected rather
than propagated. For the columns a file may carry, the rule is per column, each
matching the save path that already writes it: tags takes the file whenever the
field is present, so a present-but-empty field is an explicit clear, while
source_url and deadline take any non-empty file value and otherwise keep the
stored row. In every case an absent field means "no information", not "empty".
The tags discriminator is presence ("tags" in fields), not truthiness, which is
what makes an empty value a clear instead of a preserve. tags is canonicalized on
the way in, since the on-disk form is display text while every reader's predicate
needs the lowercase comma-joined form.
reindex() has no callers today, so nothing triggers this yet. It is kept rather
than deleted because it is the only index-rebuild path in the codebase and two
migrations, v005_tasks_fts and v023_tasks_fts_slug, both name it as how FTS
content gets filled.
Thirty-three tests are added, one behaviour per affected column and per blank
field position, so a partial fix cannot pass. Against the unfixed tree
twenty-three fail and ten pass, the ten being controls that assert unchanged
behaviour; with the fix all thirty-three pass. Two of them needed a **Status:**
line in their fixture to be meaningful at all: without one, the unfixed code
reaches the correct answer through its directory default, so the assertion was
decided by the default rather than by the rule under test. Every preservation
test also pins a file-derived value, because reindex logs and continues past a
per-file exception, so a total no-op satisfies "the value did not change" on its
own.
Coverage is pinned rather than asserted. Sixteen mutants, each weakening exactly
one clause of the change, are all killed by at least one test, and each of the
ten base-passing controls is killed by at least one mutant, so no shipped test is
unpinned. The mutant that skips every row before it is upserted kills all
twenty-six reindex tests, the seven survivors being the parser tests, which never
call reindex. An unmutated control ran green before and after the matrix.
The full suite reports 7 failed, 2966 passed against 7 failed, 2933 passed at
base, with byte-identical sorted FAILED-name sets and exactly thirty-three more
passes. The seven pre-existing failures are six in an LLM-dependent memory date
sweep and one timezone assertion; neither file references this code.
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
2 participants
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.
Problem
Completing a task moves its Markdown file and calls
upsert_task()to refresh the SQLite index. Both completion paths supplied only the new path, status, title, and content.upsert_task()has replacement semantics, so the omitted values were written back as defaults. Completing a task therefore cleared its source, source URL, deadline, and tags. The missing tags also removed them from full-text search.Fix
Carry the metadata from the existing database row into the completion upsert in both
task_done_handler()andTaskManager.mark_done(). This keeps the change local to completion and leaves the intended replacement behavior ofupsert_task()unchanged.Verification
Added regression coverage for both completion paths that verifies:
memory/tasks/done/129 passedacross the task completion, task status, and database suites.