Phase A: memo-log redesign (add/todo split, <date>:<hash> refs) - #29
Merged
Conversation
Reframe the CLI around a memo-first daily log instead of TODO management: add becomes memo-only, TODOs move under sava todo, and unfinished TODOs carry forward automatically on first touch of the day instead of via an explicit command.
Nesting start/end under todo made a TODO whose content is literally "start" or "end" impossible to create, since cobra treats a matching subcommand name as a command, not positional content. Record this as a guardrail: add and todo must never gain subcommands. Also add the Phase A code review report (English + Japanese translation).
Move start/end back to independent top-level commands so a TODO whose content is literally "start" or "end" can be created again. While touching Start/End, also confirm only after logfile.Update succeeds (not before) and collapse duplicate hashes passed to end so a repeated hash no longer produces a misleading "already finished" error.
Add/Todo/Tag called view.Added/TagsUpdated only after both logfile.Update and the follow-up index.Rebuild succeeded, so a durably persisted item showed no confirmation if the (rebuildable, cache-only) index rebuild failed afterward. Confirm right after the real write succeeds instead, independent of the index step.
newAddCommand bound its one flag to a plain local variable while the
sibling newTodoCommand bundled its two into a TodoOptions struct, an
inconsistency between two constructors that do the same kind of job.
Give Add an AddOptions{ Tags []string } too so both follow the same
pattern.
Del could show "Deleted!!" for one item while log.Delete silently removed every item sharing that hash, since hash generation had no uniqueness check. Fix both ends: Add retries a fresh hash until it doesn't collide with an existing item in the log, and Delete now finds and removes only the first match (returning it, with a not-found error otherwise, matching Finish/Start/AddTags/RemoveTags) instead of filtering out every match. This also lets command.Del call log.Delete directly, removing the now-redundant findItem helper that duplicated lookup's scan loop.
Add and Todo repeated the same "persist, confirm, rebuild index if tagged" tail. Extract it into one persistNewItem helper. The surrounding "only call AddTags if len(tags) > 0" guards were also removed: AddTags is already a no-op on empty tags, so calling it unconditionally is equivalent and persistNewItem can decide whether to rebuild the index by checking the item's own Tags instead.
diff resolving a bare hash via index.json's hash-to-date map assumed hashes are unique across every day, but the collision retry only guarantees that within a single day's log. Require the date up front instead. del also moves from today-only to searching the last 30 days by default (matching clear's retention window), erroring on an ambiguous match instead of guessing.
Diff now takes two <date>:<hash> refs and resolves each with a direct logfile.Stat, so it never needs a global hash-to-date lookup. index.json's Hashes field, command.lookup, and Diff's stale-index self-heal retry are all removed since lookup was their only caller. Del gains a <date>:<hash> form too, plus a bare-hash search across the last 30 days (all days with --deep): zero matches is a not-found error, one match deletes it, more than one aborts with an ambiguous- hash error instead of guessing. internal/log.hashExists is exported as HashExists so Del's search reuses it instead of a fourth hand-rolled scan loop.
Unify Add/Todo/Tag's persist tail into one persistItem helper driven by an explicit tagsChanged bool instead of inferring from the item's tag count, closing the trap where removing an item's last tag would skip the index rebuild it still needs (new regression test verified to fail against the naive version). Also: point a stale test comment at the test that actually exists, note End's atomicity doc comment doesn't cover concurrent processes, share the duplicated "break index.Rebuild" test setup, and collapse view.go's Deleted/FinishedTask/StartedTask into one confirmItem helper.
HashExists and Delete each scanned items for a matching hash independently; both now call a shared indexOf(items, hash) int helper. uniqueID also drops the package-level generateID var in favor of taking the generator as an explicit next func() string parameter, so tests can force a collision by passing a stub directly instead of swapping and defer-restoring global state.
Two spots in the design doc predate later decisions and would mislead a future Phase C implementation: the freeze write-command list still grouped del with add/todo/tag as "only touches today," which stopped being true once del gained cross-day search; and carriedFrom was still specified as a bare hash, reintroducing the cross-day ambiguity the <date>:<hash> redesign was written to eliminate.
parseRef accepted a ref like ":abc12345" as valid (found=true from strings.Cut, empty date), so diff/del silently resolved it against today's log instead of rejecting the malformed input. Verified live that sava del ":<hash>" previously deleted today's item instead of erroring, and that the fix rejects it while leaving the item intact.
This was referenced Aug 17, 2026
Closed
Merged
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.
Summary
Phase A of the memo-log redesign (see
docs/memo-log-redesign.md): the CLIshifts from TODO-management-first to memo-log-first.
addbecomes memo-only; TODOs move to a newsava todocommand (-sstart-immediately,
-ttags)sava start/sava endstay independent top-level commands (todomust never gain subcommands — nesting
start/endunder it made a TODOwhose content is literally "start"/"end" impossible to create; found and
reverted during review)
add/delnow confirm what they changed, including the new item's hashendaccepts multiple hashes atomically, dedupedsava diffnow requires<date>:<hash>for both sides instead ofresolving a bare hash through
index.json's hash→date map (that field,plus the lookup/self-heal machinery, is removed — hash uniqueness is only
ever guaranteed within a single day's log, so a global reverse index was
unsound)
sava delsearches the last 30 days by default (clear's retentionwindow), accepts
<date>:<hash>directly, and--deepto searcheverything; an ambiguous match (same hash, multiple days) errors instead
of guessing
This branch went through three rounds of high-effort multi-agent code
review; findings and fixes are recorded in
docs/reports/(untracked,kept local per request) and in the commit messages themselves. A handful of
low-severity reuse/efficiency findings from the third round were
deliberately left as accepted tech debt rather than fixed, to avoid an
unbounded review loop on ever-smaller surface area.
Test plan
gofmt -l .cleango vet ./...cleango test ./...— all packages passadd/todo/start/end/tag/diff/del(including
--deepand ambiguous cross-day hash) against a sandbox$HOMEparseRefempty-datebug found in review round 3