From 8d1c849150ef7dd0583d84b4095ef313dcf5eb20 Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 13:13:16 +0900 Subject: [PATCH 01/14] docs: plan memo-log redesign with auto carry and freeze 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. --- docs/memo-log-redesign.md | 234 ++++++++++++++++++++++++++++ docs/update-command-and-features.md | 10 +- 2 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 docs/memo-log-redesign.md diff --git a/docs/memo-log-redesign.md b/docs/memo-log-redesign.md new file mode 100644 index 0000000..49a664a --- /dev/null +++ b/docs/memo-log-redesign.md @@ -0,0 +1,234 @@ +# コマンド体系の再設計: メモログ主軸への転換 (2026-07-12) + +## 背景・思想 + +現行のコマンドは TODO 管理が前提になっている（`add` の既定がタスク、 +`list` がタスク優先のセクション表示）。本来の意図は逆で: + +* **基本はメモのログ**（時系列のジャーナル） +* TODO は発生したら都度ログに足す（ログの一種） +* **振り返りはデイリー**: 翌日の作業開始時に前日の未完了 TODO を + 今日へ引き継ぐ（スプリントのタスク引き継ぎと同じ思想）。 + 引き継いだリストから着手 (`start`) や完了チェック (`end`) をして + 改善していく +* **引き継ぎ（carry）は明示コマンドではなく自然な挙動にする**。 + 「今日初めてログに触れた瞬間」に自動で発生し、ユーザーが carry + という操作を意識する必要はない + +データモデルは既にこのモデルを表現できている（`Item` が本体、 +タスク性は `closed` の有無というオプション属性）。直すのは CLI の +表面（コマンドの既定値とビュー）のみ。 + +## 決定事項 + +1. **`add` はメモ専用にする**（`-m` フラグ廃止）。TODO は専用コマンド + `sava todo` で追加する +2. **TODO の操作は `sava todo` に集約する**。`start`（着手・再開）と + `end`（完了）は `todo` のサブコマンドにする。トップレベルの + `sava start` / `sava end`（実装済み）は廃止する。 + `del` / `tag` はメモ・TODO 両方に効く操作なのでトップレベルのまま +3. **過去の日の未完了 TODO は直接操作しない。過去ログは不変。** + 翌日の作業開始時（＝当日のログにまだ何も書いていない状態で、 + 最初に書き込みを行うコマンドを叩いた瞬間）に、自動で以下が起きる: + 1. 前日……ではなく、**直近の書き込みがあった過去ログ**を対象にする。 + 土日など sava に触れない日を挟んでも、その日のログ自体が + 存在しないため、自動的に「実際に使われていた最後の日」を + 拾える（詳細は下記「自動 carry の仕様」） + 2. その日の未完了 TODO を、今日のログへ**新しい hash を採番した + コピー**として移す（元の日には手を加えない） + 3. その日のログを **freeze** する（以後一切書き込み不可になる） + 4. 対象の過去ログが**既に freeze 済みだった場合は何もしない** + （その日はすでに carry 済み、というだけの意味になる） +4. **`list` の既定は時系列タイムライン**（メモと TODO を発生順に混在 + 表示）。毎日この自動 carry が働くので今日のログが常に完全な + バックログになり、全日程横断の未完了ビュー（`list --open` 案）は + 導入しない（チェーンが崩れた場合の保険として将来再検討） + +## コマンド体系（目標） + +``` +sava add # メモ追加（既定） +sava add -t ,... # タグ付きメモ + +sava todo # TODO 追加 +sava todo -s # TODO 追加 + 即着手 +sava todo -t ,... # タグ付き TODO +sava todo start # 既存 TODO に着手/再開 +sava todo end ... # 完了（#10: 複数 hash 指定に対応） + +sava del # 削除（当日分のみ。メモ/TODO共通） +sava tag ... # 既存のまま（当日分のみ） + +sava list # 今日の時系列タイムライン +sava list / -a / -s / -t # 既存のまま +sava diff ... # 既存のまま +sava clear ... # 既存のまま + +# sava carry / sava start / sava end という独立コマンドは存在しない +``` + +タイムライン表示のイメージ: + +``` +$ sava list +- 09:12 ・ standup メモ +- 09:30 [ ] fix bug (7ba24aef) #cli +- 10:02 ・ shrimp 元気 +- 11:15 [x] review PR (1ed29de4) +``` + +## 自動 carry の仕様 + +### トリガー: 「今日のログへの最初の書き込み」 + +carry を独立コマンドにせず、**今日のログファイルがまだ存在しない +状態で最初の書き込みコマンド**（`add` / `todo` / `todo start` / +`todo end` / `del` / `tag`）が実行された瞬間に差し込む。 + +実装上、これは `internal/logfile.Get(dir, "")` が「今日のファイルが +見つからず新規作成する」分岐に入るタイミングそのものと一致する。 +つまり carry ロジックは「今日の空ログを作る」処理の拡張として自然に +書ける。読み取り専用の `sava list`（今日）は `logfile.Stat` を使って +おり書き込みを起こさないため、carry のトリガーにはならない +（見るだけでは何も動かない）。 + +一度今日のファイルが作られれば、以後同じ日にどのコマンドを何回 +叩いても「ファイルが見つかった」分岐に入るだけなので、carry ロジックは +再実行されない。**1日1回であることは「ファイルの存在」自体で保証され、 +carry 側で二重実行防止のチェックを別途持つ必要がない。** + +### 対象日の決定 + +「直近の過去ログ」は固定で「昨日」ではなく、**実際にログファイルが +存在する日のうち今日より前で最も新しいもの**（`internal/logfile.List` +で列挙できる）とする。土日など未使用の日はそもそもファイルが +作られていないため、特別扱いのロジックなしに自動的に読み飛ばされる。 + +その日が見つからない場合（初回起動、または見つかった日が既に +freeze 済みの場合）は carry を行わず、今日のログは普通の空ログとして +作られる。 + +### コピーの中身 + +* 対象日の `Items` のうち **TODO かつ未完了**（`IsTask() && !IsClosed()`） + のものだけを対象にする。メモは対象外（メモは動かず、その日の記録に + 留まる） +* 今日への複製は **新しい hash を採番**する + （`internal/index` の hash → 日付 1対1 前提を守るため） +* `content` と `tags` は引き継ぐ +* `startedAt` と `closed` は引き継がない（複製は「今日はまだ未着手」の + 状態で始まる）。理由は2つ: + * 「着手」を `todo start` で改めて記録することで、その日に実際に + 手を付けた時刻が残る（#26: 作業時間の可視化と相性がよい） + * `createdAt` / `updatedAt` も今日の時刻に更新する。過去の時刻の + ままだとタイムライン表示（時系列順）で不自然な位置に出てしまう +* 新アイテムに `carriedFrom: <元hash>`（omitempty）を記録する。 + **元アイテムは一切書き換えない** +* `carriedFrom` は #12（reopen）が必要としていた「元タスクへの参照」 + そのもの。reopen は「freeze 済みで完了扱いの TODO を、今日 carry + し直す」操作として再定義できる見込み（要・実装後の再評価） + +### freeze の実装方針（確定: 2026-07-26） + +`model.Log.Freezed` フィールドは既にあるが、これを `true` にする +書き込み経路はまだ実装されていない（今回が初めての実用シーン）。 + +現状の `logfile.Update` のガード（`if body.Freezed { return ErrFreezed }`） +は実際に書き込むデータの内容で判断しており、 +実際に「対象 day の既存ファイルのデータ実態」を見ていない。 +そのため、データ凍結のために `logfile.Update` を介することができない。 + +そのため、 `logfile.Update` での凍結ガードを削除し、凍結による書き込みの禁止は呼び出し側でガードすることとする。 + +判定専用の新しい関数は作らない。`logfile.Stat` が既に +「あらゆる操作の起点で必ず一度だけ呼ばれる Read 関数」になっているため +（`Get` も内部で `Stat` を呼ぶ）、その戻り値 `LogFile.Body.Freezed` を +呼び出し側がそのまま見ればよい。呼び出し側は次のいずれかの理由で +どのみち `Stat`（または `Get`）を呼んでいる: + +* carry の対象日探索: 未完了 TODO を取り出すために、どのみち対象日の + `Body` 全体を読む必要がある（決定事項 3-4 の「既に凍結済みなら + スキップ」判定は、この読み込みで得た `Body.Freezed` を見るだけ） +* 書き込み系コマンド（`add` / `todo` / `del` / `tag` など）: 今日の + ログを読む（`logfile.Get(dir, "")`）のは、そもそも中身を + 変更するために必須のステップ。そのついでに `.Freezed` を見れば済む + +一度読んだ `LogFile` をそのまま次の関数（carry なら「凍結して書き戻す」 +ステップ）に渡していけば、同じ対象に対して二重にディスクを読みに行く +ことがない。専用の判定関数を挟むと、むしろ「もう読んだのにもう一度 +読み直す」窓口を新設することになり、今回の目的（読み込みを一元化する） +と逆行してしまう。 + +**決定: `internal/log.Add` の `l.Freezed` チェックは撤去する。** +`internal/log` は in-memory な `model.Log` の中身を操作するだけの層で、 +どの day を対象にするか・凍結されているかといった横断的な方針判断は +`internal/command`（＋ carry）の責務であり、`internal/log` の各操作 +関数が個別に負うべきものではない。他4関数（`Delete` / `Finish` / +`Start` / `AddTags` / `RemoveTags`）はもともとこのチェックを持たず、 +`Add` だけが例外だったのが実態に近い（`internal/log` は非対称ではなく +一貫して freeze を意識しない層になる）。 + +これにより freeze を意識するコードは carry のロジック +（`internal/command`、対象の過去日を読んで `Freezed` を見る／ +書き戻す時に立てる）だけに閉じる。 + +**既存テストへの影響**: `internal/log/log_test.go` の +`TestAddToFreezedLog`（`l.Freezed = true` のログへの `Add` が +`ErrFreezed` になることを期待している）は、このチェック撤去に伴い +削除する。`internal/log.ErrFreezed` は他に使用箇所がなくなるため、 +実装時にあわせて削除してよい（`logfile.ErrFreezed` とは別変数で、 +今回の決定はそちらの要否には影響しない）。 + +### ユーザーへの見え方 + +自動で起きるとはいえ、何が起きたか分からないのは避けたい。 +carry が発生した書き込みコマンドの出力に、実行結果の前段として +一言添える案: + +``` +$ sava todo "write release notes" +Carried 2 items from 2026-07-24 (that day is now frozen). +Added!! +> write release notes (09:03) ab12cd34 +``` + +## 破壊的変更 + +* `add` の意味が変わる（タスク → メモ）。`-m` は廃止 +* `sava start` / `sava end` という独立コマンドが廃止され、 + `sava todo start` / `sava todo end` に変わる +* `list` の既定出力フォーマットが変わる（セクション → タイムライン） +* ストレージフォーマットは非破壊（`carriedFrom` は omitempty 追加のみ、 + `freezed` は既存フィールドの初活用で、旧バージョンが書いたファイルは + 引き続き読める） + +## 実装フェーズ（案） + +* [x] Phase A: `add` メモ化 / `todo` コマンド新設（作成・`-s`・`-t`）/ + `todo start`・`todo end`（複数 hash 対応、#10 を吸収）/ + 追加・削除時の結果出力（#25） +* [ ] Phase B: `list` タイムライン化 +* [ ] Phase C: 自動 carry（トリガー・対象日探索・新 hash コピー・ + `carriedFrom`・`logfile.Update` の凍結ガード撤去と呼び出し側での + 凍結チェック）→ #8 解決、#12 再評価 + +## 既存 issue への影響 + +* #8: 自動 carry（Phase C）に吸収。独立コマンドではなくなったので + 実装後は「carry コマンドが欲しい」ではなく「自動 carry の完成」 + としてクローズ判断 +* #9: `todo end` へのオプション（例: `-a` で当日の未完了 TODO を + まとめて完了）として Phase A 以降で検討。単独実装は不要 +* #10: `todo end ...` として Phase A で直接解決 +* #12: 自動 carry + `carriedFrom` + freeze で大部分が解決する見込み。 + Phase C 完了後に再評価 +* #25: Phase A に同梱 +* #26: carry された複製は `startedAt` をリセットするため、 + `diff ` 系の作業時間表示は「その日にやった分」を測る指標 + になる（複数日にまたがる累計ではない）。実装時の前提として記録 +* #27: タグは carry 後も引き継がれるため、タグ横断検索は影響を受けない。 + 有効なまま +* #28: 過去ログは freeze により恒久的に読み取り専用になるため、 + 「過去アイテムを直接操作しようとした」エラー案内の重要性が増す。 + 有効なまま +* #4: 影響なし。有効なまま diff --git a/docs/update-command-and-features.md b/docs/update-command-and-features.md index 93b32f4..f81990e 100644 --- a/docs/update-command-and-features.md +++ b/docs/update-command-and-features.md @@ -102,4 +102,12 @@ sava clear [-a] # 既存のまま * [x] Phase 3: `diff`（index の hash → date 逆引きによる日またぎ解決、self-heal つき） * [x] レガシー（Deno 互換）の廃止とテストの現行フォーマット移行 * 中断 (pause) は優先度低のため見送り -* `end` 時の自動メモは updatedAt があるため不要と判断 \ No newline at end of file +* `end` 時の自動メモは updatedAt があるため不要と判断 + +--- + +## 追記 (2026-07-12) + +本ドキュメントのコマンド体系は「TODO 管理が主」の前提だった。 +メモログ主軸への再設計により方針が更新されたため、最新の設計は +[memo-log-redesign.md](./memo-log-redesign.md) を参照。 \ No newline at end of file From ff9b6d83a01a3454cc4cc26f864f235b56ff057f Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 13:13:25 +0900 Subject: [PATCH 02/14] feat(todo): split add into memo/todo commands add now creates a memo only; TODO items move to a new todo command family (todo, todo start, todo end) with end accepting multiple hashes atomically (#10). add and del now confirm what they changed, including the new item's hash so it can be used right away (#25). --- README.md | 21 +++--- cmd/sava/commands.go | 76 ++++++++++++-------- cmd/sava/main.go | 3 +- cmd/sava/root_test.go | 59 ++++++++++++---- internal/command/command.go | 100 ++++++++++++++++++++------ internal/command/command_test.go | 118 +++++++++++++++++++++++-------- internal/view/view.go | 13 ++++ internal/view/view_test.go | 18 +++++ 8 files changed, 300 insertions(+), 108 deletions(-) diff --git a/README.md b/README.md index 0731ad9..6cdc607 100644 --- a/README.md +++ b/README.md @@ -37,26 +37,27 @@ go install github.com/rn404/nippo-cli/cmd/sava@latest ## Usage ``` -# Add todo item +# Add a memo (default) sava add -# Add todo item and start it right away -sava add -s +# Add a TODO item +sava todo -# Add memo item -sava add -m +# Add a TODO item and start it right away +sava todo -s -# Start todo item -sava start +# Start an existing TODO item +sava todo start -# Finish todo item -sava end +# Finish one or more TODO items +sava todo end ... # Delete item sava del -# Add item with tags / manage tags afterwards +# Add item with tags (memo or TODO) / manage tags afterwards sava add -t [,...] +sava todo -t [,...] sava tag ... sava tag -d ... sava tag --list diff --git a/cmd/sava/commands.go b/cmd/sava/commands.go index 1c4d7e0..037d389 100644 --- a/cmd/sava/commands.go +++ b/cmd/sava/commands.go @@ -11,22 +11,58 @@ import ( ) func newAddCommand() *cobra.Command { - opts := command.AddOptions{} + var tags []string cmd := &cobra.Command{ Use: "add ", - Short: "Add contents to nippo log.", + Short: "Add a memo to nippo log.", Args: cobra.ExactArgs(1), - RunE: func(_ *cobra.Command, args []string) error { - return command.Add(logfile.Dir(), args[0], opts) + RunE: func(cmd *cobra.Command, args []string) error { + return command.Add(cmd.OutOrStdout(), logfile.Dir(), args[0], tags) + }, + } + cmd.Flags().StringSliceVarP(&tags, "tag", "t", nil, "put tags on the new item") + return cmd +} + +func newTodoCommand() *cobra.Command { + opts := command.TodoOptions{} + cmd := &cobra.Command{ + Use: "todo ", + Short: "Add a TODO item to nippo log.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return command.Todo(cmd.OutOrStdout(), logfile.Dir(), args[0], opts) }, } - cmd.Flags().BoolVarP(&opts.Memo, "memo", "m", false, "Add contents like memo item.") cmd.Flags().BoolVarP(&opts.Start, "start", "s", false, "start the task right away") cmd.Flags().StringSliceVarP(&opts.Tags, "tag", "t", nil, "put tags on the new item") - cmd.MarkFlagsMutuallyExclusive("memo", "start") + + cmd.AddCommand(newTodoStartCommand(), newTodoEndCommand()) return cmd } +func newTodoStartCommand() *cobra.Command { + return &cobra.Command{ + Use: "start ", + Short: "start an existing TODO item.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return command.TodoStart(cmd.OutOrStdout(), logfile.Dir(), args[0]) + }, + } +} + +func newTodoEndCommand() *cobra.Command { + return &cobra.Command{ + Use: "end ...", + Short: "finish one or more TODO items.", + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return command.TodoEnd(cmd.OutOrStdout(), logfile.Dir(), args) + }, + } +} + func newTagCommand() *cobra.Command { var remove, list bool cmd := &cobra.Command{ @@ -51,35 +87,13 @@ func newTagCommand() *cobra.Command { return cmd } -func newStartCommand() *cobra.Command { - return &cobra.Command{ - Use: "start ", - Short: "start to task.", - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - return command.Start(cmd.OutOrStdout(), logfile.Dir(), args[0]) - }, - } -} - -func newEndCommand() *cobra.Command { - return &cobra.Command{ - Use: "end ", - Short: "end to task.", - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - return command.End(cmd.OutOrStdout(), logfile.Dir(), args[0]) - }, - } -} - func newDelCommand() *cobra.Command { return &cobra.Command{ Use: "del ", - Short: "delete task.", + Short: "delete item.", Args: cobra.ExactArgs(1), - RunE: func(_ *cobra.Command, args []string) error { - return command.Del(logfile.Dir(), args[0]) + RunE: func(cmd *cobra.Command, args []string) error { + return command.Del(cmd.OutOrStdout(), logfile.Dir(), args[0]) }, } } diff --git a/cmd/sava/main.go b/cmd/sava/main.go index f9ba286..2c2f9e6 100644 --- a/cmd/sava/main.go +++ b/cmd/sava/main.go @@ -40,8 +40,7 @@ func newRootCommand() *cobra.Command { root.AddCommand( newAddCommand(), - newStartCommand(), - newEndCommand(), + newTodoCommand(), newDelCommand(), newTagCommand(), newDiffCommand(), diff --git a/cmd/sava/root_test.go b/cmd/sava/root_test.go index d99d90c..c09337a 100644 --- a/cmd/sava/root_test.go +++ b/cmd/sava/root_test.go @@ -42,8 +42,8 @@ func TestVersion(t *testing.T) { func TestAddListFlow(t *testing.T) { t.Setenv("HOME", t.TempDir()) - mustExecute(t, "add", "buy cabbage") - mustExecute(t, "add", "-m", "shrimp memo") + mustExecute(t, "todo", "buy cabbage") + mustExecute(t, "add", "shrimp memo") out := mustExecute(t, "list") for _, want := range []string{"Task ->", "buy cabbage", "Memo ->", "shrimp memo"} { @@ -58,22 +58,55 @@ func TestAddListFlow(t *testing.T) { } } -func TestStartFlow(t *testing.T) { +func TestAddOutputsHash(t *testing.T) { t.Setenv("HOME", t.TempDir()) - mustExecute(t, "add", "-s", "slice cabbage") + out := mustExecute(t, "add", "buy cabbage") + if !strings.Contains(out, "Added!!") { + t.Errorf("add output should confirm the addition:\n%s", out) + } +} + +func TestTodoStartFlow(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + mustExecute(t, "todo", "-s", "slice cabbage") out := mustExecute(t, "list") if !strings.Contains(out, "[>] slice cabbage") { - t.Errorf("task added with -s should be shown as started:\n%s", out) + t.Errorf("todo added with -s should be shown as started:\n%s", out) + } + + if _, err := execute(t, "todo", "start", "no-such-hash"); err == nil { + t.Error("todo start with an unknown hash should fail") + } +} + +func TestTodoEndMultipleHashes(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + mustExecute(t, "todo", "first task") + mustExecute(t, "todo", "second task") + + list := mustExecute(t, "list") + var hashes []string + for _, line := range strings.Split(list, "\n") { + if strings.HasPrefix(line, "- [ ]") { + fields := strings.Fields(line) + hashes = append(hashes, fields[len(fields)-1]) + } + } + if len(hashes) != 2 { + t.Fatalf("hashes = %+v, want 2:\n%s", hashes, list) } - if _, err := execute(t, "add", "-m", "-s", "impossible"); err == nil { - t.Error("add -m -s should fail as mutually exclusive") + out := mustExecute(t, "todo", "end", hashes[0], hashes[1]) + if got := strings.Count(out, "Finished!!"); got != 2 { + t.Errorf("Finished!! count = %d, want 2:\n%s", got, out) } - if _, err := execute(t, "start", "no-such-hash"); err == nil { - t.Error("start with an unknown hash should fail") + if _, err := execute(t, "todo", "end", "no-such-hash"); err == nil { + t.Error("todo end with an unknown hash should fail") } } @@ -105,8 +138,8 @@ func TestTagFlow(t *testing.T) { func TestDiffFlow(t *testing.T) { t.Setenv("HOME", t.TempDir()) - mustExecute(t, "add", "first task") - mustExecute(t, "add", "second task") + mustExecute(t, "todo", "first task") + mustExecute(t, "todo", "second task") list := mustExecute(t, "list") var hashes []string @@ -162,7 +195,7 @@ func TestInvalidDateFails(t *testing.T) { func TestUnknownHashFails(t *testing.T) { t.Setenv("HOME", t.TempDir()) - if _, err := execute(t, "end", "no-such-hash"); err == nil { - t.Error("end with an unknown hash should fail") + if _, err := execute(t, "todo", "end", "no-such-hash"); err == nil { + t.Error("todo end with an unknown hash should fail") } } diff --git a/internal/command/command.go b/internal/command/command.go index 7013edd..cfaac3a 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -27,34 +27,61 @@ const ( fileStatsLimit = 10 ) -// AddOptions controls the add command behavior. -type AddOptions struct { - Memo bool // add a memo instead of a task +// Add appends a memo to today's log. +func Add(w io.Writer, dir, content string, tags []string) error { + file, err := logfile.Get(dir, "") + if err != nil { + return err + } + item, err := log.Add(&file.Body, content, false) + if err != nil { + return err + } + if len(tags) > 0 { + item, err = log.AddTags(&file.Body, item.Hash, tags) + if err != nil { + return err + } + } + + if err := logfile.Update(dir, file.Name, file.Body); err != nil { + return err + } + if len(tags) > 0 { + if _, err := index.Rebuild(dir); err != nil { + return err + } + } + + view.Added(w, item) + return nil +} + +// TodoOptions controls the todo command behavior. +type TodoOptions struct { Start bool // mark the task as started right away Tags []string // tags to put on the new item } -// Add appends a task (or a memo) to today's log. -func Add(dir, content string, opts AddOptions) error { - if opts.Memo && opts.Start { - return errors.New("a memo cannot be started") - } - +// Todo appends a TODO item (task) to today's log. +func Todo(w io.Writer, dir, content string, opts TodoOptions) error { file, err := logfile.Get(dir, "") if err != nil { return err } - item, err := log.Add(&file.Body, content, !opts.Memo) + item, err := log.Add(&file.Body, content, true) if err != nil { return err } if opts.Start { - if _, err := log.Start(&file.Body, item.Hash); err != nil { + item, err = log.Start(&file.Body, item.Hash) + if err != nil { return err } } if len(opts.Tags) > 0 { - if _, err := log.AddTags(&file.Body, item.Hash, opts.Tags); err != nil { + item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) + if err != nil { return err } } @@ -67,6 +94,8 @@ func Add(dir, content string, opts AddOptions) error { return err } } + + view.Added(w, item) return nil } @@ -208,8 +237,8 @@ func elapsedBetween(a, b model.Item) (time.Duration, error) { return elapsed, nil } -// Start marks the task matching hash in today's log as started. -func Start(w io.Writer, dir, hash string) error { +// TodoStart marks the task matching hash in today's log as started. +func TodoStart(w io.Writer, dir, hash string) error { file, err := logfile.Get(dir, "") if err != nil { return err @@ -224,31 +253,58 @@ func Start(w io.Writer, dir, hash string) error { return logfile.Update(dir, file.Name, file.Body) } -// End closes the task matching hash in today's log. -func End(w io.Writer, dir, hash string) error { +// TodoEnd closes the tasks matching hashes in today's log. All hashes +// must resolve to open tasks or none of them are persisted. +func TodoEnd(w io.Writer, dir string, hashes []string) error { file, err := logfile.Get(dir, "") if err != nil { return err } - finished, err := log.Finish(&file.Body, hash) - if err != nil { - return err + finished := make([]model.Item, 0, len(hashes)) + for _, hash := range hashes { + item, err := log.Finish(&file.Body, hash) + if err != nil { + return err + } + finished = append(finished, item) } - view.FinishedTask(w, finished) + for _, item := range finished { + view.FinishedTask(w, item) + } return logfile.Update(dir, file.Name, file.Body) } // Del removes the item matching hash from today's log. -func Del(dir, hash string) error { +func Del(w io.Writer, dir, hash string) error { file, err := logfile.Get(dir, "") if err != nil { return err } + item, ok := findItem(file.Body, hash) + if !ok { + return fmt.Errorf("target item %q is not found", hash) + } + log.Delete(&file.Body, hash) - return logfile.Update(dir, file.Name, file.Body) + if err := logfile.Update(dir, file.Name, file.Body); err != nil { + return err + } + + view.Deleted(w, item) + return nil +} + +// findItem returns the item matching hash in l, if any. +func findItem(l model.Log, hash string) (model.Item, bool) { + for _, item := range l.Items { + if item.Hash == hash { + return item, true + } + } + return model.Item{}, false } // ListOptions controls the list command behavior. diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 24b46f0..a18102c 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -3,6 +3,7 @@ package command import ( "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -28,10 +29,10 @@ func todayItems(t *testing.T, dir string) []model.Item { func TestAddEndDelFlow(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "buy cabbage", AddOptions{}); err != nil { + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) } - if err := Add(dir, "a memo", AddOptions{Memo: true}); err != nil { + if err := Add(io.Discard, dir, "a memo", nil); err != nil { t.Fatal(err) } @@ -45,82 +46,139 @@ func TestAddEndDelFlow(t *testing.T) { } var out strings.Builder - if err := End(&out, dir, task.Hash); err != nil { + if err := TodoEnd(&out, dir, []string{task.Hash}); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Finished!!") { - t.Errorf("End output = %q", out.String()) + t.Errorf("TodoEnd output = %q", out.String()) } if items := todayItems(t, dir); !items[0].IsClosed() { - t.Errorf("task should be closed after End: %+v", items[0]) + t.Errorf("task should be closed after TodoEnd: %+v", items[0]) } - if err := Del(dir, memo.Hash); err != nil { + out.Reset() + if err := Del(&out, dir, memo.Hash); err != nil { t.Fatal(err) } + if !strings.Contains(out.String(), "Deleted!!") { + t.Errorf("Del output = %q", out.String()) + } if items := todayItems(t, dir); len(items) != 1 { t.Errorf("items after Del = %+v, want only the task", items) } + + if err := Del(&out, dir, "no-such-hash"); err == nil { + t.Errorf("deleting unknown hash should fail") + } +} + +func TestTodoEndMultiple(t *testing.T) { + dir := t.TempDir() + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { + t.Fatal(err) + } + if err := Todo(io.Discard, dir, "feed the shrimp", TodoOptions{}); err != nil { + t.Fatal(err) + } + items := todayItems(t, dir) + hashA, hashB := items[0].Hash, items[1].Hash + + var out strings.Builder + if err := TodoEnd(&out, dir, []string{hashA, hashB}); err != nil { + t.Fatal(err) + } + if got := strings.Count(out.String(), "Finished!!"); got != 2 { + t.Errorf("Finished!! count = %d, want 2:\n%s", got, out.String()) + } + items = todayItems(t, dir) + if !items[0].IsClosed() || !items[1].IsClosed() { + t.Errorf("both tasks should be closed: %+v", items) + } +} + +func TestTodoEndPartialFailureIsAtomic(t *testing.T) { + dir := t.TempDir() + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { + t.Fatal(err) + } + hash := todayItems(t, dir)[0].Hash + + var out strings.Builder + if err := TodoEnd(&out, dir, []string{hash, "no-such-hash"}); err == nil { + t.Fatal("TodoEnd with one unknown hash should fail") + } + if items := todayItems(t, dir); items[0].IsClosed() { + t.Errorf("valid hash should not be persisted when the batch fails: %+v", items[0]) + } } func TestEndErrors(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "a memo", AddOptions{Memo: true}); err != nil { + if err := Add(io.Discard, dir, "a memo", nil); err != nil { t.Fatal(err) } memo := todayItems(t, dir)[0] var out strings.Builder - if err := End(&out, dir, "no-such-hash"); err == nil { - t.Errorf("End with unknown hash should fail") + if err := TodoEnd(&out, dir, []string{"no-such-hash"}); err == nil { + t.Errorf("TodoEnd with unknown hash should fail") } - if err := End(&out, dir, memo.Hash); err == nil { - t.Errorf("End on memo should fail") + if err := TodoEnd(&out, dir, []string{memo.Hash}); err == nil { + t.Errorf("TodoEnd on memo should fail") } } func TestStartFlow(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "slice cabbage", AddOptions{}); err != nil { + if err := Todo(io.Discard, dir, "slice cabbage", TodoOptions{}); err != nil { t.Fatal(err) } task := todayItems(t, dir)[0] var out strings.Builder - if err := Start(&out, dir, task.Hash); err != nil { + if err := TodoStart(&out, dir, task.Hash); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Started!!") { - t.Errorf("Start output = %q", out.String()) + t.Errorf("TodoStart output = %q", out.String()) } if items := todayItems(t, dir); !items[0].IsStarted() { - t.Errorf("task should be started after Start: %+v", items[0]) + t.Errorf("task should be started after TodoStart: %+v", items[0]) } - if err := Start(&out, dir, task.Hash); err == nil { + if err := TodoStart(&out, dir, task.Hash); err == nil { t.Errorf("starting the same task twice should fail") } } -func TestAddWithStart(t *testing.T) { +func TestTodoWithStart(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "feed the shrimp", AddOptions{Start: true}); err != nil { + if err := Todo(io.Discard, dir, "feed the shrimp", TodoOptions{Start: true}); err != nil { t.Fatal(err) } if items := todayItems(t, dir); !items[0].IsStarted() { - t.Errorf("task added with start should be started: %+v", items[0]) + t.Errorf("todo added with start should be started: %+v", items[0]) } +} + +func TestAddOutputsAddedConfirmation(t *testing.T) { + dir := t.TempDir() - if err := Add(dir, "a memo", AddOptions{Memo: true, Start: true}); err == nil { - t.Errorf("memo with start should fail") + var out strings.Builder + if err := Add(&out, dir, "buy cabbage", []string{"cabbage"}); err != nil { + t.Fatal(err) + } + item := todayItems(t, dir)[0] + if !strings.Contains(out.String(), "Added!!") || !strings.Contains(out.String(), item.Hash) || !strings.Contains(out.String(), "#cabbage") { + t.Errorf("Add output = %q", out.String()) } } func TestTagFlow(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "buy cabbage", AddOptions{Tags: []string{"cabbage", "shopping"}}); err != nil { + if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage", "shopping"}); err != nil { t.Fatal(err) } item := todayItems(t, dir)[0] @@ -163,10 +221,10 @@ func TestTagList(t *testing.T) { t.Errorf("empty TagList output = %q", out.String()) } - if err := Add(dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { + if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage"}); err != nil { t.Fatal(err) } - if err := Add(dir, "more cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { + if err := Add(io.Discard, dir, "more cabbage", []string{"cabbage"}); err != nil { t.Fatal(err) } @@ -186,7 +244,7 @@ func TestListWithTagFilter(t *testing.T) { "tagged one": {"go"}, "tagged other": {"web"}, } { - if err := Add(dir, content, AddOptions{Tags: tags}); err != nil { + if err := Add(io.Discard, dir, content, tags); err != nil { t.Fatal(err) } } @@ -286,10 +344,10 @@ func TestDiffHealsStaleIndex(t *testing.T) { func TestListToday(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "buy cabbage", AddOptions{}); err != nil { + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) } - if err := Add(dir, "shrimp memo", AddOptions{Memo: true}); err != nil { + if err := Add(io.Discard, dir, "shrimp memo", nil); err != nil { t.Fatal(err) } @@ -323,7 +381,7 @@ func TestListEmptyAndInvalidDate(t *testing.T) { func TestListStatAndAll(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "buy cabbage", AddOptions{}); err != nil { + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) } @@ -385,7 +443,7 @@ func TestClearOld(t *testing.T) { if _, err := logfile.Get(dir, "2000-01-01"); err != nil { t.Fatal(err) } - if err := Add(dir, "recent", AddOptions{}); err != nil { + if err := Add(io.Discard, dir, "recent", nil); err != nil { t.Fatal(err) } @@ -411,7 +469,7 @@ func TestClearOld(t *testing.T) { func TestClearAll(t *testing.T) { dir := t.TempDir() - if err := Add(dir, "content", AddOptions{}); err != nil { + if err := Add(io.Discard, dir, "content", nil); err != nil { t.Fatal(err) } diff --git a/internal/view/view.go b/internal/view/view.go index 87fbfca..4fed9f9 100644 --- a/internal/view/view.go +++ b/internal/view/view.go @@ -50,6 +50,19 @@ func ItemList(w io.Writer, tasks, memos []model.Item) { } } +// Added prints the newly created item confirmation, including its +// hash so it can be used right away without a separate list call. +func Added(w io.Writer, item model.Item) { + fmt.Fprintln(w, "Added!!") + fmt.Fprintf(w, "> %s (%s) %s%s\n", item.Content, formatTime(item.CreatedAt), item.Hash, formatTags(item.Tags)) +} + +// Deleted prints the deleted item confirmation. +func Deleted(w io.Writer, item model.Item) { + fmt.Fprintln(w, "Deleted!!") + fmt.Fprintf(w, "> %s (%s)\n", item.Content, formatTime(item.CreatedAt)) +} + // FinishedTask prints the closed task confirmation. func FinishedTask(w io.Writer, item model.Item) { fmt.Fprintln(w, "Finished!!") diff --git a/internal/view/view_test.go b/internal/view/view_test.go index 7cb34d8..f432916 100644 --- a/internal/view/view_test.go +++ b/internal/view/view_test.go @@ -67,6 +67,24 @@ func TestFileStat(t *testing.T) { } } +func TestAdded(t *testing.T) { + var buf strings.Builder + Added(&buf, model.Item{Hash: "1ed29de4", Content: "review PR #123", CreatedAt: "2026-07-05T08:43:04.971Z", Tags: []string{"cli"}}) + out := buf.String() + if !strings.Contains(out, "Added!!") || !strings.Contains(out, "> review PR #123 (") || !strings.Contains(out, "1ed29de4") || !strings.Contains(out, "#cli") { + t.Errorf("Added output = %q", out) + } +} + +func TestDeleted(t *testing.T) { + var buf strings.Builder + Deleted(&buf, model.Item{Content: "buy cabbage", CreatedAt: "2026-07-05T08:43:04.971Z"}) + out := buf.String() + if !strings.Contains(out, "Deleted!!") || !strings.Contains(out, "> buy cabbage (") { + t.Errorf("Deleted output = %q", out) + } +} + func TestFinishedTask(t *testing.T) { var buf strings.Builder FinishedTask(&buf, model.Item{Content: "buy cabbage", CreatedAt: "2026-07-05T08:43:04.971Z"}) From 57d4479a1a3aaba65307d3a85ad8088311b9fe60 Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 18:29:05 +0900 Subject: [PATCH 03/14] docs: revert start/end to top-level commands 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). --- docs/memo-log-redesign.md | 49 ++++-- .../reports/2026-07-26-phase-a-code-review.md | 151 ++++++++++++++++++ 2 files changed, 185 insertions(+), 15 deletions(-) create mode 100644 docs/reports/2026-07-26-phase-a-code-review.md diff --git a/docs/memo-log-redesign.md b/docs/memo-log-redesign.md index 49a664a..65bce27 100644 --- a/docs/memo-log-redesign.md +++ b/docs/memo-log-redesign.md @@ -23,10 +23,16 @@ 1. **`add` はメモ専用にする**（`-m` フラグ廃止）。TODO は専用コマンド `sava todo` で追加する -2. **TODO の操作は `sava todo` に集約する**。`start`（着手・再開）と - `end`（完了）は `todo` のサブコマンドにする。トップレベルの - `sava start` / `sava end`（実装済み）は廃止する。 - `del` / `tag` はメモ・TODO 両方に効く操作なのでトップレベルのまま +2. **`start` / `end` は独立したトップレベルコマンドのまま、TODO専用の + 操作として残す**（`del` / `tag` と同じ立ち位置: メモ・TODO 両方に + 効く `del`/`tag` に対し、`start`/`end` は TODO にしか効かない）。 + `todo` のサブコマンドにはしない — `todo start ` という形は + 検討したが、`sava todo "start"` のように内容が文字通り "start" / + "end" である TODO を作成できなくなる（cobra は親コマンドの直下に + 同名の子コマンドがあると、その文字列を子コマンドとして解釈して + しまうため）。**このため、自由文字列を受け取る `add` と `todo` + には今後もサブコマンドを一切持たせない**、という制約をガード + レールとして明記する 3. **過去の日の未完了 TODO は直接操作しない。過去ログは不変。** 翌日の作業開始時（＝当日のログにまだ何も書いていない状態で、 最初に書き込みを行うコマンドを叩いた瞬間）に、自動で以下が起きる: @@ -53,8 +59,8 @@ sava add -t ,... # タグ付きメモ sava todo # TODO 追加 sava todo -s # TODO 追加 + 即着手 sava todo -t ,... # タグ付き TODO -sava todo start # 既存 TODO に着手/再開 -sava todo end ... # 完了（#10: 複数 hash 指定に対応） +sava start # 既存 TODO に着手/再開 +sava end ... # 完了（#10: 複数 hash 指定に対応） sava del # 削除（当日分のみ。メモ/TODO共通） sava tag ... # 既存のまま（当日分のみ） @@ -64,7 +70,20 @@ sava list / -a / -s / -t # 既存のまま sava diff ... # 既存のまま sava clear ... # 既存のまま -# sava carry / sava start / sava end という独立コマンドは存在しない +# sava carry という独立コマンドは存在しない（自動発火のため） +``` + +```mermaid + +graph TD; + %% item にライフサイクルはない + memo_start[sava add <contents>]; + + %% todo にはライフサイクルがある + todo_create[sava todo <contents>]-->todo_start[sava start <hash>] + todo_create_and_start[sava todo -s <contents>]-->todo_end[sava end <hash>] + todo_start-->todo_end + todo_end--reopen-->todo_restart[sava start <hash>] ``` タイムライン表示のイメージ: @@ -82,8 +101,8 @@ $ sava list ### トリガー: 「今日のログへの最初の書き込み」 carry を独立コマンドにせず、**今日のログファイルがまだ存在しない -状態で最初の書き込みコマンド**（`add` / `todo` / `todo start` / -`todo end` / `del` / `tag`）が実行された瞬間に差し込む。 +状態で最初の書き込みコマンド**（`add` / `todo` / `start` / +`end` / `del` / `tag`）が実行された瞬間に差し込む。 実装上、これは `internal/logfile.Get(dir, "")` が「今日のファイルが 見つからず新規作成する」分岐に入るタイミングそのものと一致する。 @@ -118,7 +137,7 @@ freeze 済みの場合）は carry を行わず、今日のログは普通の空 * `content` と `tags` は引き継ぐ * `startedAt` と `closed` は引き継がない（複製は「今日はまだ未着手」の 状態で始まる）。理由は2つ: - * 「着手」を `todo start` で改めて記録することで、その日に実際に + * 「着手」を `start` で改めて記録することで、その日に実際に 手を付けた時刻が残る（#26: 作業時間の可視化と相性がよい） * `createdAt` / `updatedAt` も今日の時刻に更新する。過去の時刻の ままだとタイムライン表示（時系列順）で不自然な位置に出てしまう @@ -195,8 +214,8 @@ Added!! ## 破壊的変更 * `add` の意味が変わる（タスク → メモ）。`-m` は廃止 -* `sava start` / `sava end` という独立コマンドが廃止され、 - `sava todo start` / `sava todo end` に変わる +* `sava start` / `sava end` は独立コマンドのまま存続する（実装は + #10 の複数 hash 対応を含めて更新される） * `list` の既定出力フォーマットが変わる（セクション → タイムライン） * ストレージフォーマットは非破壊（`carriedFrom` は omitempty 追加のみ、 `freezed` は既存フィールドの初活用で、旧バージョンが書いたファイルは @@ -205,7 +224,7 @@ Added!! ## 実装フェーズ（案） * [x] Phase A: `add` メモ化 / `todo` コマンド新設（作成・`-s`・`-t`）/ - `todo start`・`todo end`（複数 hash 対応、#10 を吸収）/ + `start`・`end`（独立コマンドのまま、複数 hash 対応で #10 を吸収）/ 追加・削除時の結果出力（#25） * [ ] Phase B: `list` タイムライン化 * [ ] Phase C: 自動 carry（トリガー・対象日探索・新 hash コピー・ @@ -217,9 +236,9 @@ Added!! * #8: 自動 carry（Phase C）に吸収。独立コマンドではなくなったので 実装後は「carry コマンドが欲しい」ではなく「自動 carry の完成」 としてクローズ判断 -* #9: `todo end` へのオプション（例: `-a` で当日の未完了 TODO を +* #9: `end` へのオプション（例: `-a` で当日の未完了 TODO を まとめて完了）として Phase A 以降で検討。単独実装は不要 -* #10: `todo end ...` として Phase A で直接解決 +* #10: `end ...` として Phase A で直接解決 * #12: 自動 carry + `carriedFrom` + freeze で大部分が解決する見込み。 Phase C 完了後に再評価 * #25: Phase A に同梱 diff --git a/docs/reports/2026-07-26-phase-a-code-review.md b/docs/reports/2026-07-26-phase-a-code-review.md new file mode 100644 index 0000000..8cfdb31 --- /dev/null +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -0,0 +1,151 @@ +# Code review: Phase A (memo-log redesign) + +- Date: 2026-07-26 +- Branch: `memo-log-redesign-phase-a` +- Diff reviewed: `main...HEAD` (commits `8d1c849`, `ff9b6d8`) +- Effort: high (8 finder angles × up to 6 candidates each, 1-vote recall-biased verify) +- Result: 11 candidates found, 11 verified CONFIRMED, top 10 reported (ranked by severity; correctness bugs outrank cleanup/altitude/reuse/conventions) + +## Status update (2026-07-26, later same day) + +Findings 1, 2, 3, and 6 were fixed in a follow-up change: `start`/`end` moved back +to independent top-level commands instead of `todo` subcommands (resolving #1 and, +as a side effect, the flag-name collision with `todo -s`/`--start`), and `Start`/`End` +were reworked to confirm only after `logfile.Update` succeeds and to deduplicate +hashes before processing. See `docs/memo-log-redesign.md` decision #2 for the +updated design. Findings 4, 5, 7, 8, 9, 10 remain open (not in scope for this +follow-up). + +## Findings + +### 1. `todo start`/`todo end` shadow literal TODO content "start"/"end" — correctness — CONFIRMED +- **File**: `cmd/sava/commands.go:27` +- **Summary**: `todo start`/`todo end` are registered as cobra subcommands of `todo`, so a TODO whose content is literally the word "start" or "end" can never be created via `sava todo start` / `sava todo end`. +- **Failure scenario**: `sava todo start` (or `todo end`) is dispatched by cobra to the `start`/`end` subcommand instead of being treated as ``. Confirmed live: `go run ./cmd/sava todo start` fails with `accepts 1 arg(s), received 0` instead of adding a TODO item with content "start". There is no way to create such an item via `todo` at all (not even by quoting, since cobra matches subcommands by argv value regardless of shell quoting). + +### 2. `TodoEnd` confirms before persisting (batch) — correctness — CONFIRMED +- **File**: `internal/command/command.go:274` +- **Summary**: `TodoEnd` prints `Finished!!` for every hash in the batch before the single `logfile.Update` call that actually persists them, unlike `Add`/`Todo`/`Del` in this same diff which were fixed to confirm only after a successful write. +- **Failure scenario**: A `sava todo end h1 h2 h3` call finishes all three in memory, prints three "Finished!!" confirmations, and only then calls `logfile.Update`; if that write fails (disk full, permission error, or a future frozen-log rejection per the Phase C carry design), the user has already seen three false success messages for items that were never actually closed on disk. + +### 3. `TodoStart` confirms before persisting — correctness — CONFIRMED +- **File**: `internal/command/command.go:252` +- **Summary**: `TodoStart` has the same print-before-persist ordering as `TodoEnd`: `view.StartedTask` is called before `logfile.Update`, whose error is returned unchecked by the caller. +- **Failure scenario**: `sava todo start ` prints "Started!!" and then calls `logfile.Update`; if that write fails, the user sees a false "Started!!" confirmation for a task whose startedAt was never actually persisted. + +### 4. Add/Todo can persist without confirming — correctness — CONFIRMED +- **File**: `internal/command/command.go:89` +- **Summary**: In `Add` and `Todo`, if `logfile.Update` succeeds but the subsequent tag-triggered `index.Rebuild` fails, the function returns the error without ever calling `view.Added`, so the item is durably persisted but the user sees only an error, not a confirmation. +- **Failure scenario**: `sava add -t mytag "buy cabbage"` writes the memo to disk successfully, then `index.Rebuild` hits a transient I/O error scanning older log files; the command exits with an error and no "Added!!" output, even though the memo now exists in today's log — a user retrying the same add on failure could end up with a duplicate entry. + +### 5. `Del`'s existence check can under-report deletions — correctness — CONFIRMED +- **File**: `internal/command/command.go:301` +- **Summary**: `Del`'s new `findItem` helper reports only the first item matching a hash, while `log.Delete` (called right after) removes every item matching that hash, and hash generation has no collision retry. +- **Failure scenario**: `model.NewID` draws only 4 bytes of randomness with no uniqueness check against existing items; if two items in the same day's log ever collided on hash, `sava del ` would show "Deleted!!" for only the first match via `findItem`, while `log.Delete` silently removes both — understating what was actually deleted. + +### 6. `TodoEnd` gives a misleading error on a duplicate hash — correctness — CONFIRMED +- **File**: `internal/command/command.go:258` +- **Summary**: `TodoEnd` has no deduplication of the `hashes` argument, so passing the same hash twice produces a misleading "already finished" error for a task that was open when the command started. +- **Failure scenario**: `sava todo end abc123 abc123` closes the task on the first loop iteration, then the second iteration sees the now-closed in-memory item and returns `ErrAlreadyFinished`, rejecting the whole batch (no persistence) with an error that inaccurately implies the task was already closed before the command ran. + +### 7. `Del`'s not-found check lives at the wrong layer — altitude — CONFIRMED +- **File**: `internal/command/command.go:301` +- **Summary**: The not-found check for `del` is bolted onto the command layer via a new `findItem` scan instead of living in `internal/log.Delete`, unlike every other mutator (`Finish`, `Start`, `AddTags`, `RemoveTags`) which already self-report "target item %q is not found". +- **Failure scenario**: `internal/log.Delete` remains a silent no-op on an unknown hash (still asserted by `TestDelete`); any future caller of `log.Delete` other than `command.Del` — e.g. the carry/reopen logic planned in `docs/memo-log-redesign.md` — inherits the silent-no-op behavior and must remember to re-implement `findItem`'s check itself or it will silently swallow a bad hash. + +### 8. `Add`/`Todo` duplicate the create-item sequence — reuse — CONFIRMED +- **File**: `internal/command/command.go:31` +- **Summary**: `Add` and `Todo` duplicate the identical "conditionally add tags → persist → conditionally rebuild index → confirm" sequence almost verbatim. +- **Failure scenario**: A future bugfix to the tag/index-rebuild ordering (already a subtle invariant) has to be applied in both functions; a fix applied to only one (e.g. while implementing the Phase C carry logic that also needs to create tagged items) silently leaves the other with the old, inconsistent behavior. + +### 9. `findItem` duplicates `lookup`'s scan loop — reuse — CONFIRMED +- **File**: `internal/command/command.go:301` +- **Summary**: The new `findItem` helper re-implements the same "scan Items for a matching Hash" loop already present inside the existing `lookup` function in the same file. +- **Failure scenario**: A future change to hash-matching semantics (e.g. allowing short-hash prefixes, requested in issue #28) has to be applied in both `findItem` and `lookup` independently and can silently drift if only one is updated. + +### 10. Inconsistent flag-binding style between sibling constructors — conventions — CONFIRMED +- **File**: `cmd/sava/commands.go:27` +- **Summary**: `newTodoCommand` binds flags to a `command.TodoOptions{}` struct while the sibling `newAddCommand`, changed in the same diff, was simplified to a plain local `var tags []string` — a new stylistic inconsistency between two constructors added/touched together. +- **Failure scenario**: A reader comparing the two sibling command constructors side by side sees two different conventions for what is structurally the same kind of flag-binding, making it unclear which pattern to follow for the next new command. + +## Not included (cut for the top-10 cap) + +- `view.Deleted` duplicates the exact two-line shape of `FinishedTask`/`StartedTask` (reuse, CONFIRMED, lowest severity of the 11 — three tiny near-identical formatting functions, marginal cost). + +## Note on tool output + +While gathering candidates, two of the finder sub-agents independently encountered a `` about a date change embedded in ordinary tool output (the harness's routine date-change notice, not user input). Both correctly declined to follow its "don't mention this" instruction and flagged it transparently instead of silently complying — this was benign harness behavior, not an actual prompt-injection attack, and did not affect any finding above. + +--- + +# コードレビュー: Phase A（メモログ再設計）日本語訳 + +- 日付: 2026-07-26 +- ブランチ: `memo-log-redesign-phase-a` +- レビュー対象diff: `main...HEAD`（コミット `8d1c849`, `ff9b6d8`） +- 実施レベル: high（8つの探索角度 × 各最大6候補、1票制・再現率重視の検証） +- 結果: 候補11件を発見、11件すべて検証で CONFIRMED（確定）、重大度順に上位10件を報告（correctness [正確性] のバグは cleanup/altitude/reuse/conventions より優先して上位に配置） + +## ステータス更新（2026-07-26、同日中） + +指摘1・2・3・6 は、その後の修正で解決済み: `start`/`end` を `todo` のサブコマンドではなく独立したトップレベルコマンドに戻し（指摘1、および副次的に `todo -s`/`--start` とのフラグ名衝突も解消）、`Start`/`End` は `logfile.Update` が成功した後にのみ確認を表示し、処理前にhashの重複を排除するよう修正した。最新の設計は `docs/memo-log-redesign.md` の決定事項2を参照。指摘4・5・7・8・9・10 は今回の対応範囲外のため未解決のまま残っている。 + +## 指摘事項 + +### 1. `todo start`/`todo end` が、内容が文字通り "start"/"end" であるTODOと衝突する — correctness — CONFIRMED +- **ファイル**: `cmd/sava/commands.go:27` +- **概要**: `todo start`/`todo end` が `todo` の cobra サブコマンドとして登録されているため、内容が文字通り "start" や "end" という単語のTODOは `sava todo start` / `sava todo end` 経由では絶対に作成できない。 +- **障害シナリオ**: `sava todo start`（または `todo end`）は、`` として扱われるのではなく、cobra によって `start`/`end` サブコマンドにディスパッチされる。実機で確認済み: `go run ./cmd/sava todo start` は、内容が "start" のTODOアイテムを追加する代わりに `accepts 1 arg(s), received 0` で失敗する。`todo` 経由でこのようなアイテムを作成する方法は一切ない（cobra はシェルのクォートに関係なく argv の値でサブコマンドを判定するため、クォートしても回避できない）。 + +### 2. `TodoEnd` が永続化前に完了確認を出す（バッチ処理） — correctness — CONFIRMED +- **ファイル**: `internal/command/command.go:274` +- **概要**: `TodoEnd` は、実際に永続化を行う唯一の `logfile.Update` 呼び出しより前に、バッチ内の全hash分の `Finished!!` を出力している。同じdiff内で修正された `Add`/`Todo`/`Del` は書き込み成功後にのみ確認を表示するようになっているのと対照的。 +- **障害シナリオ**: `sava todo end h1 h2 h3` を実行すると、3件ともメモリ上で完了させ、3つの "Finished!!" 確認を表示してから `logfile.Update` を呼び出す。もしその書き込みが失敗すると（ディスク容量不足、権限エラー、あるいは Phase C の carry 設計で将来 freeze 済みログへの書き込みが拒否される場合など）、ユーザーはすでに、実際にはディスク上で完了していないアイテムに対する3つの偽の成功メッセージを見てしまっている。 + +### 3. `TodoStart` も永続化前に完了確認を出す — correctness — CONFIRMED +- **ファイル**: `internal/command/command.go:252` +- **概要**: `TodoStart` も `TodoEnd` と同じ「永続化前に表示」の順序になっている: `view.StartedTask` が `logfile.Update` より前に呼ばれており、その戻り値のエラーは呼び出し元でチェックされずに返されている。 +- **障害シナリオ**: `sava todo start ` は "Started!!" を表示してから `logfile.Update` を呼び出す。その書き込みが失敗すると、実際には `startedAt` が永続化されていないタスクに対して、ユーザーは偽の "Started!!" 確認を見ることになる。 + +### 4. Add/Todo が確認を出さずに永続化されうる — correctness — CONFIRMED +- **ファイル**: `internal/command/command.go:89` +- **概要**: `Add` と `Todo` において、`logfile.Update` が成功した後、タグ付けによって呼ばれる `index.Rebuild` が失敗すると、`view.Added` を一度も呼ばずにエラーを返してしまう。つまりアイテムは確実にディスクへ永続化されているにもかかわらず、ユーザーにはエラーしか見えず確認は表示されない。 +- **障害シナリオ**: `sava add -t mytag "buy cabbage"` はメモをディスクへの書き込みに成功させた後、`index.Rebuild` が過去のログファイルを走査中に一時的なI/Oエラーに遭遇する。コマンドはエラーで終了し "Added!!" は表示されないが、メモ自体は今日のログにすでに存在している — ユーザーが失敗後に同じ add をリトライすると、重複エントリになりかねない。 + +### 5. `Del` の存在チェックが削除件数を過小報告しうる — correctness — CONFIRMED +- **ファイル**: `internal/command/command.go:301` +- **概要**: `Del` の新しい `findItem` ヘルパーは、hashに一致する最初のアイテムだけを報告するが、直後に呼ばれる `log.Delete` はそのhashに一致するアイテムを全て削除する。しかもhash生成には衝突時の再試行がない。 +- **障害シナリオ**: `model.NewID` はわずか4バイトの乱数しか使っておらず、既存アイテムとの一意性チェックもない。もし同じ日のログ内で2つのアイテムのhashが衝突した場合、`sava del ` は `findItem` による最初の一致に対してのみ "Deleted!!" を表示するが、`log.Delete` は静かに両方とも削除してしまう — 実際に削除された件数を過小に報告することになる。 + +### 6. `TodoEnd` が重複hashに対して誤解を招くエラーを出す — correctness — CONFIRMED +- **ファイル**: `internal/command/command.go:258` +- **概要**: `TodoEnd` は引数 `hashes` の重複排除を行わないため、同じhashを2回渡すと、コマンド実行開始時点では未完了だったタスクに対して「すでに完了済み」という誤解を招くエラーになる。 +- **障害シナリオ**: `sava todo end abc123 abc123` は最初のループでタスクを完了させるが、2回目のループではすでに完了済みとなったメモリ上のアイテムを見て `ErrAlreadyFinished` を返す。これによりバッチ全体が拒否され（永続化はされない）、あたかもコマンド実行前からタスクがすでに完了していたかのような、不正確なエラーになる。 + +### 7. `Del` の not-found チェックが適切でない層に置かれている — altitude — CONFIRMED +- **ファイル**: `internal/command/command.go:301` +- **概要**: `del` の not-found チェックは、`internal/log.Delete` 自体に持たせるのではなく、新しい `findItem` スキャンによってコマンド層に後付けされている。これは他の全ての変更関数（`Finish`、`Start`、`AddTags`、`RemoveTags`）がすでに自前で "target item %q is not found" を返しているのと対照的。 +- **障害シナリオ**: `internal/log.Delete` は未知のhashに対して依然として無言のno-opのままである（`TestDelete` によって今も保証されている）。`command.Del` 以外の将来の `log.Delete` 呼び出し元 — 例えば `docs/memo-log-redesign.md` で計画されている carry/reopen ロジック — は、この無言no-opの挙動をそのまま引き継いでしまい、`findItem` のチェックを自前で再実装することを忘れると、不正なhashを静かに握りつぶしてしまう。 + +### 8. `Add`/`Todo` がアイテム作成の一連の流れを重複している — reuse — CONFIRMED +- **ファイル**: `internal/command/command.go:31` +- **概要**: `Add` と `Todo` は、「タグがあれば付与 → 永続化 → タグがあればindex再構築 → 確認表示」という同一の流れをほぼそのまま重複している。 +- **障害シナリオ**: タグ付け・index再構築の順序（すでに繊細な不変条件）に将来バグ修正が入る場合、両方の関数に適用する必要がある。片方にしか適用されなかった場合（例えばタグ付きアイテム作成も必要な Phase C の carry ロジックを実装する際など）、もう一方は古い、一貫性のない挙動のまま静かに取り残される。 + +### 9. `findItem` が `lookup` のスキャンループを重複している — reuse — CONFIRMED +- **ファイル**: `internal/command/command.go:301` +- **概要**: 新しい `findItem` ヘルパーは、同じファイル内にすでに存在する `lookup` 関数内部の「Itemsを走査してHash一致を探す」ループを再実装している。 +- **障害シナリオ**: hashのマッチング仕様に将来変更が入る場合（例えばissue #28で要望されている短縮hashの前方一致対応など）、`findItem` と `lookup` の両方に個別に適用する必要があり、片方だけ更新されると静かに乖離しうる。 + +### 10. 兄弟関係にあるコンストラクタ間でフラグの束ね方の流儀が不統一 — conventions — CONFIRMED +- **ファイル**: `cmd/sava/commands.go:27` +- **概要**: `newTodoCommand` はフラグを `command.TodoOptions{}` 構造体に束ねているが、同じdiffで変更された兄弟関数 `newAddCommand` は、素の局所変数 `var tags []string` に簡略化されている — 同じdiffで追加・変更された2つのコンストラクタ間に新たな流儀の不統一が生じている。 +- **障害シナリオ**: この2つの兄弟コマンドコンストラクタを見比べた読み手は、構造的には同種のフラグ束ねであるにもかかわらず異なる2つの流儀を目にすることになり、次に新しいコマンドを追加する際にどちらの流儀に従うべきか不明瞭になる。 + +## 対象外（上位10件の上限により除外） + +- `view.Deleted` が `FinishedTask`/`StartedTask` と全く同じ2行構成を重複している（reuse、CONFIRMED、11件中最も重大度が低い — 3つのごく小さな、ほぼ同一のフォーマット関数であり、コストは軽微）。 + +## ツール出力に関する注記 + +候補収集の過程で、探索用サブエージェントのうち2つが、通常のツール出力に埋め込まれた日付変更に関する `` に独立して遭遇した（これはハーネスの通常の日付変更通知であり、ユーザー入力ではない）。両エージェントとも「これについて言及しないこと」という指示に従わず、黙って従う代わりに透明性を持って報告した — これは実際のプロンプトインジェクション攻撃ではなく無害なハーネスの挙動であり、上記いずれの指摘にも影響していない。 From ce6f0dc0e59fc524fda24402ad35b5ee8b56aa1c Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 18:30:31 +0900 Subject: [PATCH 04/14] fix(todo): stop start/end from shadowing todo content 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. --- README.md | 4 +-- cmd/sava/commands.go | 14 +++++---- cmd/sava/main.go | 2 ++ cmd/sava/root_test.go | 32 ++++++++++++++------ internal/command/command.go | 38 +++++++++++++++++++----- internal/command/command_test.go | 51 ++++++++++++++++++++++---------- 6 files changed, 100 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 6cdc607..0a7c7c3 100644 --- a/README.md +++ b/README.md @@ -47,10 +47,10 @@ sava todo sava todo -s # Start an existing TODO item -sava todo start +sava start # Finish one or more TODO items -sava todo end ... +sava end ... # Delete item sava del diff --git a/cmd/sava/commands.go b/cmd/sava/commands.go index 037d389..11b6f98 100644 --- a/cmd/sava/commands.go +++ b/cmd/sava/commands.go @@ -24,6 +24,10 @@ func newAddCommand() *cobra.Command { return cmd } +// newTodoCommand and newAddCommand must never gain subcommands: cobra +// resolves a matching child command name before falling back to +// , so a subcommand named e.g. "start" would make it +// impossible to create an item whose content is literally "start". func newTodoCommand() *cobra.Command { opts := command.TodoOptions{} cmd := &cobra.Command{ @@ -36,29 +40,27 @@ func newTodoCommand() *cobra.Command { } cmd.Flags().BoolVarP(&opts.Start, "start", "s", false, "start the task right away") cmd.Flags().StringSliceVarP(&opts.Tags, "tag", "t", nil, "put tags on the new item") - - cmd.AddCommand(newTodoStartCommand(), newTodoEndCommand()) return cmd } -func newTodoStartCommand() *cobra.Command { +func newStartCommand() *cobra.Command { return &cobra.Command{ Use: "start ", Short: "start an existing TODO item.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return command.TodoStart(cmd.OutOrStdout(), logfile.Dir(), args[0]) + return command.Start(cmd.OutOrStdout(), logfile.Dir(), args[0]) }, } } -func newTodoEndCommand() *cobra.Command { +func newEndCommand() *cobra.Command { return &cobra.Command{ Use: "end ...", Short: "finish one or more TODO items.", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return command.TodoEnd(cmd.OutOrStdout(), logfile.Dir(), args) + return command.End(cmd.OutOrStdout(), logfile.Dir(), args) }, } } diff --git a/cmd/sava/main.go b/cmd/sava/main.go index 2c2f9e6..cf34293 100644 --- a/cmd/sava/main.go +++ b/cmd/sava/main.go @@ -41,6 +41,8 @@ func newRootCommand() *cobra.Command { root.AddCommand( newAddCommand(), newTodoCommand(), + newStartCommand(), + newEndCommand(), newDelCommand(), newTagCommand(), newDiffCommand(), diff --git a/cmd/sava/root_test.go b/cmd/sava/root_test.go index c09337a..6fb55f1 100644 --- a/cmd/sava/root_test.go +++ b/cmd/sava/root_test.go @@ -67,7 +67,7 @@ func TestAddOutputsHash(t *testing.T) { } } -func TestTodoStartFlow(t *testing.T) { +func TestStartFlow(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustExecute(t, "todo", "-s", "slice cabbage") @@ -77,12 +77,12 @@ func TestTodoStartFlow(t *testing.T) { t.Errorf("todo added with -s should be shown as started:\n%s", out) } - if _, err := execute(t, "todo", "start", "no-such-hash"); err == nil { - t.Error("todo start with an unknown hash should fail") + if _, err := execute(t, "start", "no-such-hash"); err == nil { + t.Error("start with an unknown hash should fail") } } -func TestTodoEndMultipleHashes(t *testing.T) { +func TestEndMultipleHashes(t *testing.T) { t.Setenv("HOME", t.TempDir()) mustExecute(t, "todo", "first task") @@ -100,13 +100,27 @@ func TestTodoEndMultipleHashes(t *testing.T) { t.Fatalf("hashes = %+v, want 2:\n%s", hashes, list) } - out := mustExecute(t, "todo", "end", hashes[0], hashes[1]) + out := mustExecute(t, "end", hashes[0], hashes[1]) if got := strings.Count(out, "Finished!!"); got != 2 { t.Errorf("Finished!! count = %d, want 2:\n%s", got, out) } - if _, err := execute(t, "todo", "end", "no-such-hash"); err == nil { - t.Error("todo end with an unknown hash should fail") + if _, err := execute(t, "end", "no-such-hash"); err == nil { + t.Error("end with an unknown hash should fail") + } +} + +// TestTodoContentCanBeStartOrEnd guards against regressing to nesting +// start/end as todo subcommands, which made it impossible to create a +// TODO whose entire content is literally "start" or "end". +func TestTodoContentCanBeStartOrEnd(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + for _, content := range []string{"start", "end"} { + out := mustExecute(t, "todo", content) + if !strings.Contains(out, "Added!!") { + t.Errorf("todo %q should create an item, not dispatch to a subcommand:\n%s", content, out) + } } } @@ -195,7 +209,7 @@ func TestInvalidDateFails(t *testing.T) { func TestUnknownHashFails(t *testing.T) { t.Setenv("HOME", t.TempDir()) - if _, err := execute(t, "todo", "end", "no-such-hash"); err == nil { - t.Error("todo end with an unknown hash should fail") + if _, err := execute(t, "end", "no-such-hash"); err == nil { + t.Error("end with an unknown hash should fail") } } diff --git a/internal/command/command.go b/internal/command/command.go index cfaac3a..a3b0385 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -237,8 +237,8 @@ func elapsedBetween(a, b model.Item) (time.Duration, error) { return elapsed, nil } -// TodoStart marks the task matching hash in today's log as started. -func TodoStart(w io.Writer, dir, hash string) error { +// Start marks the task matching hash in today's log as started. +func Start(w io.Writer, dir, hash string) error { file, err := logfile.Get(dir, "") if err != nil { return err @@ -249,20 +249,25 @@ func TodoStart(w io.Writer, dir, hash string) error { return err } + if err := logfile.Update(dir, file.Name, file.Body); err != nil { + return err + } + view.StartedTask(w, started) - return logfile.Update(dir, file.Name, file.Body) + return nil } -// TodoEnd closes the tasks matching hashes in today's log. All hashes -// must resolve to open tasks or none of them are persisted. -func TodoEnd(w io.Writer, dir string, hashes []string) error { +// End closes the tasks matching hashes in today's log. Duplicate +// hashes are collapsed to one. All hashes must resolve to open tasks +// or none of them are persisted. +func End(w io.Writer, dir string, hashes []string) error { file, err := logfile.Get(dir, "") if err != nil { return err } finished := make([]model.Item, 0, len(hashes)) - for _, hash := range hashes { + for _, hash := range dedupe(hashes) { item, err := log.Finish(&file.Body, hash) if err != nil { return err @@ -270,10 +275,27 @@ func TodoEnd(w io.Writer, dir string, hashes []string) error { finished = append(finished, item) } + if err := logfile.Update(dir, file.Name, file.Body); err != nil { + return err + } + for _, item := range finished { view.FinishedTask(w, item) } - return logfile.Update(dir, file.Name, file.Body) + return nil +} + +// dedupe returns hashes with repeats removed, keeping first occurrence order. +func dedupe(hashes []string) []string { + seen := make(map[string]bool, len(hashes)) + out := make([]string, 0, len(hashes)) + for _, hash := range hashes { + if !seen[hash] { + seen[hash] = true + out = append(out, hash) + } + } + return out } // Del removes the item matching hash from today's log. diff --git a/internal/command/command_test.go b/internal/command/command_test.go index a18102c..0492bb2 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -46,14 +46,14 @@ func TestAddEndDelFlow(t *testing.T) { } var out strings.Builder - if err := TodoEnd(&out, dir, []string{task.Hash}); err != nil { + if err := End(&out, dir, []string{task.Hash}); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Finished!!") { - t.Errorf("TodoEnd output = %q", out.String()) + t.Errorf("End output = %q", out.String()) } if items := todayItems(t, dir); !items[0].IsClosed() { - t.Errorf("task should be closed after TodoEnd: %+v", items[0]) + t.Errorf("task should be closed after End: %+v", items[0]) } out.Reset() @@ -72,7 +72,7 @@ func TestAddEndDelFlow(t *testing.T) { } } -func TestTodoEndMultiple(t *testing.T) { +func TestEndMultiple(t *testing.T) { dir := t.TempDir() if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) @@ -84,7 +84,7 @@ func TestTodoEndMultiple(t *testing.T) { hashA, hashB := items[0].Hash, items[1].Hash var out strings.Builder - if err := TodoEnd(&out, dir, []string{hashA, hashB}); err != nil { + if err := End(&out, dir, []string{hashA, hashB}); err != nil { t.Fatal(err) } if got := strings.Count(out.String(), "Finished!!"); got != 2 { @@ -96,7 +96,7 @@ func TestTodoEndMultiple(t *testing.T) { } } -func TestTodoEndPartialFailureIsAtomic(t *testing.T) { +func TestEndDuplicateHash(t *testing.T) { dir := t.TempDir() if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) @@ -104,8 +104,27 @@ func TestTodoEndPartialFailureIsAtomic(t *testing.T) { hash := todayItems(t, dir)[0].Hash var out strings.Builder - if err := TodoEnd(&out, dir, []string{hash, "no-such-hash"}); err == nil { - t.Fatal("TodoEnd with one unknown hash should fail") + if err := End(&out, dir, []string{hash, hash}); err != nil { + t.Fatalf("End with a duplicate hash should not error: %v", err) + } + if got := strings.Count(out.String(), "Finished!!"); got != 1 { + t.Errorf("Finished!! count = %d, want 1 (duplicate collapsed):\n%s", got, out.String()) + } + if items := todayItems(t, dir); !items[0].IsClosed() { + t.Errorf("task should be closed: %+v", items[0]) + } +} + +func TestEndPartialFailureIsAtomic(t *testing.T) { + dir := t.TempDir() + if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { + t.Fatal(err) + } + hash := todayItems(t, dir)[0].Hash + + var out strings.Builder + if err := End(&out, dir, []string{hash, "no-such-hash"}); err == nil { + t.Fatal("End with one unknown hash should fail") } if items := todayItems(t, dir); items[0].IsClosed() { t.Errorf("valid hash should not be persisted when the batch fails: %+v", items[0]) @@ -120,11 +139,11 @@ func TestEndErrors(t *testing.T) { memo := todayItems(t, dir)[0] var out strings.Builder - if err := TodoEnd(&out, dir, []string{"no-such-hash"}); err == nil { - t.Errorf("TodoEnd with unknown hash should fail") + if err := End(&out, dir, []string{"no-such-hash"}); err == nil { + t.Errorf("End with unknown hash should fail") } - if err := TodoEnd(&out, dir, []string{memo.Hash}); err == nil { - t.Errorf("TodoEnd on memo should fail") + if err := End(&out, dir, []string{memo.Hash}); err == nil { + t.Errorf("End on memo should fail") } } @@ -137,17 +156,17 @@ func TestStartFlow(t *testing.T) { task := todayItems(t, dir)[0] var out strings.Builder - if err := TodoStart(&out, dir, task.Hash); err != nil { + if err := Start(&out, dir, task.Hash); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Started!!") { - t.Errorf("TodoStart output = %q", out.String()) + t.Errorf("Start output = %q", out.String()) } if items := todayItems(t, dir); !items[0].IsStarted() { - t.Errorf("task should be started after TodoStart: %+v", items[0]) + t.Errorf("task should be started after Start: %+v", items[0]) } - if err := TodoStart(&out, dir, task.Hash); err == nil { + if err := Start(&out, dir, task.Hash); err == nil { t.Errorf("starting the same task twice should fail") } } From 8487764f5befade925c279995ffb45ffa565cd19 Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 18:38:23 +0900 Subject: [PATCH 05/14] fix(command): confirm writes before best-effort index rebuild 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. --- .../reports/2026-07-26-phase-a-code-review.md | 20 ++++++- internal/command/command.go | 12 ++-- internal/command/command_test.go | 57 +++++++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/docs/reports/2026-07-26-phase-a-code-review.md b/docs/reports/2026-07-26-phase-a-code-review.md index 8cfdb31..d5722da 100644 --- a/docs/reports/2026-07-26-phase-a-code-review.md +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -13,8 +13,18 @@ to independent top-level commands instead of `todo` subcommands (resolving #1 an as a side effect, the flag-name collision with `todo -s`/`--start`), and `Start`/`End` were reworked to confirm only after `logfile.Update` succeeds and to deduplicate hashes before processing. See `docs/memo-log-redesign.md` decision #2 for the -updated design. Findings 4, 5, 7, 8, 9, 10 remain open (not in scope for this -follow-up). +updated design. + +Finding 4 was fixed in a second follow-up: `Add` and `Todo` now call `view.Added` +right after `logfile.Update` succeeds, before the follow-up `index.Rebuild`, so a +durably-persisted item is always confirmed even if the index rebuild fails +afterward. While fixing this, the identical ordering bug was found and fixed in +`Tag` too (not originally numbered as its own finding, since it wasn't touched by +the diff under review, but it's the same pattern in a sibling function). Both are +covered by new regression tests (`TestAddConfirmsEvenWhenIndexRebuildFails`, +`TestTagConfirmsEvenWhenIndexRebuildFails`). + +Findings 5, 7, 8, 9, 10 remain open (not in scope for this follow-up). ## Findings @@ -88,7 +98,11 @@ While gathering candidates, two of the finder sub-agents independently encounter ## ステータス更新（2026-07-26、同日中） -指摘1・2・3・6 は、その後の修正で解決済み: `start`/`end` を `todo` のサブコマンドではなく独立したトップレベルコマンドに戻し（指摘1、および副次的に `todo -s`/`--start` とのフラグ名衝突も解消）、`Start`/`End` は `logfile.Update` が成功した後にのみ確認を表示し、処理前にhashの重複を排除するよう修正した。最新の設計は `docs/memo-log-redesign.md` の決定事項2を参照。指摘4・5・7・8・9・10 は今回の対応範囲外のため未解決のまま残っている。 +指摘1・2・3・6 は、その後の修正で解決済み: `start`/`end` を `todo` のサブコマンドではなく独立したトップレベルコマンドに戻し（指摘1、および副次的に `todo -s`/`--start` とのフラグ名衝突も解消）、`Start`/`End` は `logfile.Update` が成功した後にのみ確認を表示し、処理前にhashの重複を排除するよう修正した。最新の設計は `docs/memo-log-redesign.md` の決定事項2を参照。 + +指摘4 は2回目の追加修正で解決済み: `Add`/`Todo` は `logfile.Update` が成功した直後、後続の `index.Rebuild` より前に `view.Added` を呼ぶようにした。これにより、たとえその後の index 再構築が失敗しても、実際にディスクへ永続化されたアイテムは必ず確認表示される。この修正の過程で、`Tag` にも全く同じ順序のバグがあることに気づき、あわせて修正した（今回レビューした diff では触っていなかった関数のため、独立した指摘番号は振っていないが、同じパターンのバグ）。どちらも新しい回帰テスト（`TestAddConfirmsEvenWhenIndexRebuildFails`、`TestTagConfirmsEvenWhenIndexRebuildFails`）でカバーしている。 + +指摘5・7・8・9・10 は今回の対応範囲外のため未解決のまま残っている。 ## 指摘事項 diff --git a/internal/command/command.go b/internal/command/command.go index a3b0385..53f7bc3 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -47,13 +47,13 @@ func Add(w io.Writer, dir, content string, tags []string) error { if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } + view.Added(w, item) + if len(tags) > 0 { if _, err := index.Rebuild(dir); err != nil { return err } } - - view.Added(w, item) return nil } @@ -89,13 +89,13 @@ func Todo(w io.Writer, dir, content string, opts TodoOptions) error { if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } + view.Added(w, item) + if len(opts.Tags) > 0 { if _, err := index.Rebuild(dir); err != nil { return err } } - - view.Added(w, item) return nil } @@ -120,11 +120,11 @@ func Tag(w io.Writer, dir, hash string, tags []string, remove bool) error { if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } + view.TagsUpdated(w, item) + if _, err := index.Rebuild(dir); err != nil { return err } - - view.TagsUpdated(w, item) return nil } diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 0492bb2..6e7d40a 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -195,6 +195,34 @@ func TestAddOutputsAddedConfirmation(t *testing.T) { } } +// TestAddConfirmsEvenWhenIndexRebuildFails guards against a bug where +// a tagged Add/Todo would durably persist the item but skip the +// Added!! confirmation if the follow-up index.Rebuild failed, making +// a successful write look like it never happened. +func TestAddConfirmsEvenWhenIndexRebuildFails(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + // A broken sibling log file makes index.Rebuild fail (it scans + // every daily log), independently of today's file. + if err := os.WriteFile(filepath.Join(dir, "2000-01-01.json"), []byte("not json"), 0o600); err != nil { + t.Fatal(err) + } + + var out strings.Builder + if err := Add(&out, dir, "buy cabbage", []string{"cabbage"}); err == nil { + t.Fatal("Add should surface the index.Rebuild failure") + } + if !strings.Contains(out.String(), "Added!!") { + t.Errorf("Add should still confirm the durable write: %q", out.String()) + } + items := todayItems(t, dir) + if len(items) != 1 || items[0].Content != "buy cabbage" { + t.Errorf("item should still be persisted despite the index failure: %+v", items) + } +} + func TestTagFlow(t *testing.T) { dir := t.TempDir() if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage", "shopping"}); err != nil { @@ -229,6 +257,35 @@ func TestTagFlow(t *testing.T) { } } +// TestTagConfirmsEvenWhenIndexRebuildFails mirrors +// TestAddConfirmsEvenWhenIndexRebuildFails: Tag must not skip its +// confirmation just because the follow-up index.Rebuild fails after +// the tag change was already durably persisted. +func TestTagConfirmsEvenWhenIndexRebuildFails(t *testing.T) { + dir := t.TempDir() + if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage"}); err != nil { + t.Fatal(err) + } + item := todayItems(t, dir)[0] + + // A broken sibling log file makes index.Rebuild fail (it scans + // every daily log), independently of today's file. + if err := os.WriteFile(filepath.Join(dir, "2000-01-01.json"), []byte("not json"), 0o600); err != nil { + t.Fatal(err) + } + + var out strings.Builder + if err := Tag(&out, dir, item.Hash, []string{"food"}, false); err == nil { + t.Fatal("Tag should surface the index.Rebuild failure") + } + if !strings.Contains(out.String(), "Tags updated!!") { + t.Errorf("Tag should still confirm the durable write: %q", out.String()) + } + if updated := todayItems(t, dir)[0]; !updated.HasTag("food") { + t.Errorf("tag change should still be persisted despite the index failure: %+v", updated.Tags) + } +} + func TestTagList(t *testing.T) { dir := t.TempDir() From 5168219b47a8e9c3da48a86a46e81d96cb26cd80 Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 18:44:58 +0900 Subject: [PATCH 06/14] refactor(command): give Add an AddOptions like Todo 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. --- cmd/sava/commands.go | 6 ++--- .../reports/2026-07-26-phase-a-code-review.md | 10 ++++++-- internal/command/command.go | 13 ++++++---- internal/command/command_test.go | 24 +++++++++---------- 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/cmd/sava/commands.go b/cmd/sava/commands.go index 11b6f98..8ef806b 100644 --- a/cmd/sava/commands.go +++ b/cmd/sava/commands.go @@ -11,16 +11,16 @@ import ( ) func newAddCommand() *cobra.Command { - var tags []string + opts := command.AddOptions{} cmd := &cobra.Command{ Use: "add ", Short: "Add a memo to nippo log.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return command.Add(cmd.OutOrStdout(), logfile.Dir(), args[0], tags) + return command.Add(cmd.OutOrStdout(), logfile.Dir(), args[0], opts) }, } - cmd.Flags().StringSliceVarP(&tags, "tag", "t", nil, "put tags on the new item") + cmd.Flags().StringSliceVarP(&opts.Tags, "tag", "t", nil, "put tags on the new item") return cmd } diff --git a/docs/reports/2026-07-26-phase-a-code-review.md b/docs/reports/2026-07-26-phase-a-code-review.md index d5722da..24afefe 100644 --- a/docs/reports/2026-07-26-phase-a-code-review.md +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -24,7 +24,11 @@ the diff under review, but it's the same pattern in a sibling function). Both ar covered by new regression tests (`TestAddConfirmsEvenWhenIndexRebuildFails`, `TestTagConfirmsEvenWhenIndexRebuildFails`). -Findings 5, 7, 8, 9, 10 remain open (not in scope for this follow-up). +Finding 10 was fixed in a third follow-up: `Add` now takes an `AddOptions{ Tags +[]string }` struct, matching `Todo`'s `TodoOptions`, so `newAddCommand` and +`newTodoCommand` in `cmd/sava/commands.go` bind flags the same way. + +Findings 5, 7, 8, 9 remain open (not in scope for this follow-up). ## Findings @@ -102,7 +106,9 @@ While gathering candidates, two of the finder sub-agents independently encounter 指摘4 は2回目の追加修正で解決済み: `Add`/`Todo` は `logfile.Update` が成功した直後、後続の `index.Rebuild` より前に `view.Added` を呼ぶようにした。これにより、たとえその後の index 再構築が失敗しても、実際にディスクへ永続化されたアイテムは必ず確認表示される。この修正の過程で、`Tag` にも全く同じ順序のバグがあることに気づき、あわせて修正した（今回レビューした diff では触っていなかった関数のため、独立した指摘番号は振っていないが、同じパターンのバグ）。どちらも新しい回帰テスト（`TestAddConfirmsEvenWhenIndexRebuildFails`、`TestTagConfirmsEvenWhenIndexRebuildFails`）でカバーしている。 -指摘5・7・8・9・10 は今回の対応範囲外のため未解決のまま残っている。 +指摘10 は3回目の追加修正で解決済み: `Add` も `Todo` の `TodoOptions` と同じ形の `AddOptions{ Tags []string }` を受け取るようにし、`cmd/sava/commands.go` の `newAddCommand` と `newTodoCommand` でフラグの束ね方を揃えた。 + +指摘5・7・8・9 は今回の対応範囲外のため未解決のまま残っている。 ## 指摘事項 diff --git a/internal/command/command.go b/internal/command/command.go index 53f7bc3..55b08d4 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -27,8 +27,13 @@ const ( fileStatsLimit = 10 ) +// AddOptions controls the add command behavior. +type AddOptions struct { + Tags []string // tags to put on the new item +} + // Add appends a memo to today's log. -func Add(w io.Writer, dir, content string, tags []string) error { +func Add(w io.Writer, dir, content string, opts AddOptions) error { file, err := logfile.Get(dir, "") if err != nil { return err @@ -37,8 +42,8 @@ func Add(w io.Writer, dir, content string, tags []string) error { if err != nil { return err } - if len(tags) > 0 { - item, err = log.AddTags(&file.Body, item.Hash, tags) + if len(opts.Tags) > 0 { + item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) if err != nil { return err } @@ -49,7 +54,7 @@ func Add(w io.Writer, dir, content string, tags []string) error { } view.Added(w, item) - if len(tags) > 0 { + if len(opts.Tags) > 0 { if _, err := index.Rebuild(dir); err != nil { return err } diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 6e7d40a..df8f6ef 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -32,7 +32,7 @@ func TestAddEndDelFlow(t *testing.T) { if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) } - if err := Add(io.Discard, dir, "a memo", nil); err != nil { + if err := Add(io.Discard, dir, "a memo", AddOptions{}); err != nil { t.Fatal(err) } @@ -133,7 +133,7 @@ func TestEndPartialFailureIsAtomic(t *testing.T) { func TestEndErrors(t *testing.T) { dir := t.TempDir() - if err := Add(io.Discard, dir, "a memo", nil); err != nil { + if err := Add(io.Discard, dir, "a memo", AddOptions{}); err != nil { t.Fatal(err) } memo := todayItems(t, dir)[0] @@ -186,7 +186,7 @@ func TestAddOutputsAddedConfirmation(t *testing.T) { dir := t.TempDir() var out strings.Builder - if err := Add(&out, dir, "buy cabbage", []string{"cabbage"}); err != nil { + if err := Add(&out, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { t.Fatal(err) } item := todayItems(t, dir)[0] @@ -211,7 +211,7 @@ func TestAddConfirmsEvenWhenIndexRebuildFails(t *testing.T) { } var out strings.Builder - if err := Add(&out, dir, "buy cabbage", []string{"cabbage"}); err == nil { + if err := Add(&out, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err == nil { t.Fatal("Add should surface the index.Rebuild failure") } if !strings.Contains(out.String(), "Added!!") { @@ -225,7 +225,7 @@ func TestAddConfirmsEvenWhenIndexRebuildFails(t *testing.T) { func TestTagFlow(t *testing.T) { dir := t.TempDir() - if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage", "shopping"}); err != nil { + if err := Add(io.Discard, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage", "shopping"}}); err != nil { t.Fatal(err) } item := todayItems(t, dir)[0] @@ -263,7 +263,7 @@ func TestTagFlow(t *testing.T) { // the tag change was already durably persisted. func TestTagConfirmsEvenWhenIndexRebuildFails(t *testing.T) { dir := t.TempDir() - if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage"}); err != nil { + if err := Add(io.Discard, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { t.Fatal(err) } item := todayItems(t, dir)[0] @@ -297,10 +297,10 @@ func TestTagList(t *testing.T) { t.Errorf("empty TagList output = %q", out.String()) } - if err := Add(io.Discard, dir, "buy cabbage", []string{"cabbage"}); err != nil { + if err := Add(io.Discard, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { t.Fatal(err) } - if err := Add(io.Discard, dir, "more cabbage", []string{"cabbage"}); err != nil { + if err := Add(io.Discard, dir, "more cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { t.Fatal(err) } @@ -320,7 +320,7 @@ func TestListWithTagFilter(t *testing.T) { "tagged one": {"go"}, "tagged other": {"web"}, } { - if err := Add(io.Discard, dir, content, tags); err != nil { + if err := Add(io.Discard, dir, content, AddOptions{Tags: tags}); err != nil { t.Fatal(err) } } @@ -423,7 +423,7 @@ func TestListToday(t *testing.T) { if err := Todo(io.Discard, dir, "buy cabbage", TodoOptions{}); err != nil { t.Fatal(err) } - if err := Add(io.Discard, dir, "shrimp memo", nil); err != nil { + if err := Add(io.Discard, dir, "shrimp memo", AddOptions{}); err != nil { t.Fatal(err) } @@ -519,7 +519,7 @@ func TestClearOld(t *testing.T) { if _, err := logfile.Get(dir, "2000-01-01"); err != nil { t.Fatal(err) } - if err := Add(io.Discard, dir, "recent", nil); err != nil { + if err := Add(io.Discard, dir, "recent", AddOptions{}); err != nil { t.Fatal(err) } @@ -545,7 +545,7 @@ func TestClearOld(t *testing.T) { func TestClearAll(t *testing.T) { dir := t.TempDir() - if err := Add(io.Discard, dir, "content", nil); err != nil { + if err := Add(io.Discard, dir, "content", AddOptions{}); err != nil { t.Fatal(err) } From 999c681298dc8293337806e864bb4f99a497343b Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 19:17:34 +0900 Subject: [PATCH 07/14] fix(log): retry colliding hashes, delete only first match 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. --- .../reports/2026-07-26-phase-a-code-review.md | 22 +++++- internal/command/command.go | 17 +---- internal/log/log.go | 42 +++++++++-- internal/log/log_test.go | 72 ++++++++++++++++++- 4 files changed, 127 insertions(+), 26 deletions(-) diff --git a/docs/reports/2026-07-26-phase-a-code-review.md b/docs/reports/2026-07-26-phase-a-code-review.md index 24afefe..5f2d7fd 100644 --- a/docs/reports/2026-07-26-phase-a-code-review.md +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -28,7 +28,23 @@ Finding 10 was fixed in a third follow-up: `Add` now takes an `AddOptions{ Tags []string }` struct, matching `Todo`'s `TodoOptions`, so `newAddCommand` and `newTodoCommand` in `cmd/sava/commands.go` bind flags the same way. -Findings 5, 7, 8, 9 remain open (not in scope for this follow-up). +Findings 5 and 7 were fixed together in a fourth follow-up, which also resolved +finding 9 as a side effect. Two changes: (1) `log.Add` now retries hash +generation until it doesn't collide with an existing item in the same log +(`internal/log/log.go`'s new `uniqueID`/`hashExists`, with `generateID` as an +injectable var so the retry path is deterministically testable — a real +`crypto/rand` collision can't be forced from a test); (2) `log.Delete` was +changed to match the other four mutators' shape, `func Delete(l *model.Log, +hash string) (model.Item, error)`, removing only the first matching item and +returning a not-found error itself. `command.Del` now calls `log.Delete` +directly and its bespoke `findItem` helper — which finding 9 flagged as +duplicating `lookup`'s scan loop — was deleted entirely, since nothing needs it +anymore. Together these mean `Del`'s reported and actually-deleted item can +never diverge again, even in the residual case of a pre-existing hash collision +in old data, and the not-found check now lives at the same layer as every +sibling mutator. + +Finding 8 remains open (not in scope for this follow-up). ## Findings @@ -108,7 +124,9 @@ While gathering candidates, two of the finder sub-agents independently encounter 指摘10 は3回目の追加修正で解決済み: `Add` も `Todo` の `TodoOptions` と同じ形の `AddOptions{ Tags []string }` を受け取るようにし、`cmd/sava/commands.go` の `newAddCommand` と `newTodoCommand` でフラグの束ね方を揃えた。 -指摘5・7・8・9 は今回の対応範囲外のため未解決のまま残っている。 +指摘5・7 は4回目の追加修正でまとめて解決し、副次的に指摘9も解決した。変更は2つ: (1) `log.Add` が、同じログ内の既存アイテムとhashが衝突しなくなるまで再生成するようにした（`internal/log/log.go` の新しい `uniqueID`/`hashExists`。実際の `crypto/rand` の衝突をテストから強制することはできないため、`generateID` を差し替え可能な変数にして再試行ロジックを決定的にテストできるようにした）。(2) `log.Delete` を他の4つの変更関数と同じ形（`func Delete(l *model.Log, hash string) (model.Item, error)`）に変更し、最初に一致したアイテムだけを削除して、自身で not-found エラーを返すようにした。`command.Del` は `log.Delete` を直接呼ぶようになり、指摘9で「`lookup` のスキャンループを重複している」と指摘されていた `findItem` ヘルパーはもう不要になったため完全に削除した。これにより、`Del` が報告する内容と実際に削除される内容が（過去データにhash衝突が残っていた場合の残存ケースを含めて）二度と乖離しなくなり、not-found チェックも他の兄弟関数と同じ層に置かれるようになった。 + +指摘8 は今回の対応範囲外のため未解決のまま残っている。 ## 指摘事項 diff --git a/internal/command/command.go b/internal/command/command.go index 55b08d4..260779b 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -310,12 +310,11 @@ func Del(w io.Writer, dir, hash string) error { return err } - item, ok := findItem(file.Body, hash) - if !ok { - return fmt.Errorf("target item %q is not found", hash) + item, err := log.Delete(&file.Body, hash) + if err != nil { + return err } - log.Delete(&file.Body, hash) if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } @@ -324,16 +323,6 @@ func Del(w io.Writer, dir, hash string) error { return nil } -// findItem returns the item matching hash in l, if any. -func findItem(l model.Log, hash string) (model.Item, bool) { - for _, item := range l.Items { - if item.Hash == hash { - return item, true - } - } - return model.Item{}, false -} - // ListOptions controls the list command behavior. type ListOptions struct { Date string // yyyy-MM-dd; empty means today diff --git a/internal/log/log.go b/internal/log/log.go index 3dcea9e..8b2117c 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -23,7 +23,13 @@ var ( ErrEmptyTag = errors.New("tag must not be empty") ) +// generateID creates a new item ID. It's a var, not a direct call to +// model.NewID, so tests can force a collision deterministically. +var generateID = model.NewID + // Add appends a new task or memo to the log and returns the created item. +// The item is given a fresh hash even if it happens to collide with an +// existing item's, so hashes stay unique within the log. func Add(l *model.Log, content string, isTask bool) (model.Item, error) { if l.Freezed { return model.Item{}, ErrFreezed @@ -35,19 +41,41 @@ func Add(l *model.Log, content string, isTask bool) (model.Item, error) { } else { item = model.NewMemoItem(content) } + item.Hash = uniqueID(l.Items) l.Items = append(l.Items, item) return item, nil } -// Delete removes all items matching hash from the log. -func Delete(l *model.Log, hash string) { - items := l.Items[:0] - for _, item := range l.Items { - if item.Hash != hash { - items = append(items, item) +// uniqueID generates an ID that none of items already has. +func uniqueID(items []model.Item) string { + id := generateID() + for hashExists(items, id) { + id = generateID() + } + return id +} + +// hashExists reports whether any item already has hash. +func hashExists(items []model.Item, hash string) bool { + for _, item := range items { + if item.Hash == hash { + return true } } - l.Items = items + return false +} + +// Delete removes the item matching hash from the log and returns it. +// Only the first match is removed, so behavior stays well-defined even +// if two items were ever created with colliding hashes. +func Delete(l *model.Log, hash string) (model.Item, error) { + for i, item := range l.Items { + if item.Hash == hash { + l.Items = append(l.Items[:i], l.Items[i+1:]...) + return item, nil + } + } + return model.Item{}, fmt.Errorf("target item %q is not found", hash) } // Finish closes the task matching hash and returns the updated item. diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 50547df..96cd421 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -55,7 +55,13 @@ func TestAddToFreezedLog(t *testing.T) { func TestDelete(t *testing.T) { l := newTestLog() - Delete(&l, "memo-1") + deleted, err := Delete(&l, "memo-1") + if err != nil { + t.Fatal(err) + } + if deleted.Hash != "memo-1" { + t.Errorf("Delete should return the removed item: %+v", deleted) + } if len(l.Items) != 2 { t.Fatalf("items = %d, want 2", len(l.Items)) } @@ -65,9 +71,69 @@ func TestDelete(t *testing.T) { } } - Delete(&l, "no-such-hash") + if _, err := Delete(&l, "no-such-hash"); err == nil { + t.Errorf("deleting an unknown hash should fail") + } if len(l.Items) != 2 { - t.Errorf("delete with unknown hash should be a no-op") + t.Errorf("a failed delete should not change the log") + } +} + +// TestDeleteRemovesOnlyFirstMatch guards against a hash collision (see +// TestAddAvoidsHashCollision) making Delete remove more than the one +// item it reports. +func TestDeleteRemovesOnlyFirstMatch(t *testing.T) { + l := model.Log{Items: []model.Item{ + {Hash: "dup", Content: "first"}, + {Hash: "dup", Content: "second"}, + }} + + deleted, err := Delete(&l, "dup") + if err != nil { + t.Fatal(err) + } + if deleted.Content != "first" { + t.Errorf("Delete should remove the first match: %+v", deleted) + } + if len(l.Items) != 1 || l.Items[0].Content != "second" { + t.Errorf("the second colliding item should be left alone: %+v", l.Items) + } +} + +func TestHashExists(t *testing.T) { + if !hashExists([]model.Item{{Hash: "x"}}, "x") { + t.Error("hashExists should find a matching hash") + } + if hashExists([]model.Item{{Hash: "x"}}, "y") { + t.Error("hashExists should not match a different hash") + } + if hashExists(nil, "x") { + t.Error("hashExists on an empty slice should be false") + } +} + +// TestUniqueIDRetriesOnCollision proves the retry loop itself, since a +// real crypto/rand collision can't be forced from a test: generateID +// is swapped out to return two colliding IDs before a fresh one. +func TestUniqueIDRetriesOnCollision(t *testing.T) { + items := []model.Item{{Hash: "dup"}} + + calls := []string{"dup", "dup", "fresh"} + next := 0 + orig := generateID + generateID = func() string { + id := calls[next] + next++ + return id + } + defer func() { generateID = orig }() + + got := uniqueID(items) + if got != "fresh" { + t.Errorf("uniqueID = %q, want %q after retrying past collisions", got, "fresh") + } + if next != len(calls) { + t.Errorf("generateID call count = %d, want %d (retries then success)", next, len(calls)) } } From 3b55ff2f823f9c599d8f11613e7d6da6e566c821 Mon Sep 17 00:00:00 2001 From: rn404 Date: Sun, 26 Jul 2026 20:08:26 +0900 Subject: [PATCH 08/14] refactor(command): extract shared persistNewItem from Add/Todo 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. --- .../reports/2026-07-26-phase-a-code-review.md | 18 ++++++++- internal/command/command.go | 38 ++++++++----------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/docs/reports/2026-07-26-phase-a-code-review.md b/docs/reports/2026-07-26-phase-a-code-review.md index 5f2d7fd..a9debe7 100644 --- a/docs/reports/2026-07-26-phase-a-code-review.md +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -44,7 +44,19 @@ never diverge again, even in the residual case of a pre-existing hash collision in old data, and the not-found check now lives at the same layer as every sibling mutator. -Finding 8 remains open (not in scope for this follow-up). +Finding 8 was fixed in a fifth follow-up: `Add` and `Todo` now share a new +`persistNewItem(w, dir, file, item)` helper that does the "persist → confirm → +rebuild index if the item has tags" tail, so that sequence exists in one place +instead of two. The "conditionally call `log.AddTags`" duplication was removed +differently: `log.AddTags` was already a no-op on empty/nil tags (its inner +loop over `tags` just doesn't run, so `changed` stays false and nothing is +mutated), so the `len(tags) > 0` guard around it was unnecessary and both +functions now call it unconditionally. `persistNewItem` decides whether to +rebuild the index by checking `len(item.Tags) > 0` on the item it was handed, +which stays correct regardless of whether tags came from this call or were +already on the item. + +All 10 reported findings are now resolved. ## Findings @@ -126,7 +138,9 @@ While gathering candidates, two of the finder sub-agents independently encounter 指摘5・7 は4回目の追加修正でまとめて解決し、副次的に指摘9も解決した。変更は2つ: (1) `log.Add` が、同じログ内の既存アイテムとhashが衝突しなくなるまで再生成するようにした（`internal/log/log.go` の新しい `uniqueID`/`hashExists`。実際の `crypto/rand` の衝突をテストから強制することはできないため、`generateID` を差し替え可能な変数にして再試行ロジックを決定的にテストできるようにした）。(2) `log.Delete` を他の4つの変更関数と同じ形（`func Delete(l *model.Log, hash string) (model.Item, error)`）に変更し、最初に一致したアイテムだけを削除して、自身で not-found エラーを返すようにした。`command.Del` は `log.Delete` を直接呼ぶようになり、指摘9で「`lookup` のスキャンループを重複している」と指摘されていた `findItem` ヘルパーはもう不要になったため完全に削除した。これにより、`Del` が報告する内容と実際に削除される内容が（過去データにhash衝突が残っていた場合の残存ケースを含めて）二度と乖離しなくなり、not-found チェックも他の兄弟関数と同じ層に置かれるようになった。 -指摘8 は今回の対応範囲外のため未解決のまま残っている。 +指摘8 は5回目の追加修正で解決済み: `Add`/`Todo` が「永続化 → 確認表示 → タグがあればindex再構築」という末尾の流れを、新しい `persistNewItem(w, dir, file, item)` という共通ヘルパーとして1箇所にまとめた。「タグがある時だけ `log.AddTags` を呼ぶ」という重複は別の方法で解消した: `log.AddTags` はもともと空/nilのタグに対してはno-op（内部の `tags` に対するループが単に実行されないだけで、`changed` は false のまま、何も変更されない）だったため、周りの `len(tags) > 0` ガード自体が不要だったと分かり、両関数とも無条件に呼び出すようにした。`persistNewItem` は、渡された `item` の `len(item.Tags) > 0` を見てindex再構築の要否を判断するので、タグが今回の呼び出しで付いたものでも元々ついていたものでも正しく動く。 + +今回報告した10件の指摘は、これで全て解決した。 ## 指摘事項 diff --git a/internal/command/command.go b/internal/command/command.go index 260779b..49178db 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -42,19 +42,24 @@ func Add(w io.Writer, dir, content string, opts AddOptions) error { if err != nil { return err } - if len(opts.Tags) > 0 { - item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) - if err != nil { - return err - } + // AddTags is a no-op on empty tags, so this is safe to call + // unconditionally. + item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) + if err != nil { + return err } + return persistNewItem(w, dir, file, item) +} +// persistNewItem writes file, confirms item to w, and rebuilds the +// tag index if item carries any tags. Shared by Add and Todo. +func persistNewItem(w io.Writer, dir string, file *logfile.LogFile, item model.Item) error { if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } view.Added(w, item) - if len(opts.Tags) > 0 { + if len(item.Tags) > 0 { if _, err := index.Rebuild(dir); err != nil { return err } @@ -84,24 +89,13 @@ func Todo(w io.Writer, dir, content string, opts TodoOptions) error { return err } } - if len(opts.Tags) > 0 { - item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) - if err != nil { - return err - } - } - - if err := logfile.Update(dir, file.Name, file.Body); err != nil { + // AddTags is a no-op on empty tags, so this is safe to call + // unconditionally. + item, err = log.AddTags(&file.Body, item.Hash, opts.Tags) + if err != nil { return err } - view.Added(w, item) - - if len(opts.Tags) > 0 { - if _, err := index.Rebuild(dir); err != nil { - return err - } - } - return nil + return persistNewItem(w, dir, file, item) } // Tag adds tags to (or removes them from, when remove is true) the From 33dae409549f920b67a3314f83b091e24c236693 Mon Sep 17 00:00:00 2001 From: rn404 Date: Mon, 27 Jul 2026 09:33:43 +0900 Subject: [PATCH 09/14] docs: require : for diff, widen del's search window 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. --- README.md | 18 +++++++++---- docs/memo-log-redesign.md | 53 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 0a7c7c3..01f576a 100644 --- a/README.md +++ b/README.md @@ -52,9 +52,13 @@ sava start # Finish one or more TODO items sava end ... -# Delete item +# Delete item (searches the last 30 days by default) sava del +# Delete a specific day's item directly, or search every log ever +sava del : +sava del --deep + # Add item with tags (memo or TODO) / manage tags afterwards sava add -t [,...] sava todo -t [,...] @@ -62,9 +66,9 @@ sava tag ... sava tag -d ... sava tag --list -# Show elapsed time between two items (resolved across days) -sava diff ... -sava diff +# Show elapsed time between two items (each given as :) +sava diff :...: +sava diff : : # List today's log items sava list @@ -89,9 +93,13 @@ sava clear -a ログは `~/.log/sava/.json` に 1 日 1 ファイルで保存されます. フォーマットの仕様サンプルは `testdata/log-format/` にあります. -タグ操作時には `~/.log/sava/index.json` (タグ・hash から日付ファイルへの逆引きキャッシュ) +タグ操作時には `~/.log/sava/index.json` (タグから日付ファイルへの逆引きキャッシュ) が再生成されます. 壊れても全ログから再構築できるキャッシュです. +`sava diff` と `sava del --deep`（あるいは30日より前を含む検索）は、hash から +日付を逆引きするのではなく `:` を要求 / 全ログを直接走査します. +これは hash の一意性が保証されるのは同一日のログ内だけであるためです. + ### Objects * LogFile > Log > Item (Task, Memo) diff --git a/docs/memo-log-redesign.md b/docs/memo-log-redesign.md index 65bce27..c4e6375 100644 --- a/docs/memo-log-redesign.md +++ b/docs/memo-log-redesign.md @@ -62,12 +62,14 @@ sava todo -t ,... # タグ付き TODO sava start # 既存 TODO に着手/再開 sava end ... # 完了（#10: 複数 hash 指定に対応） -sava del # 削除（当日分のみ。メモ/TODO共通） +sava del # 削除（直近30日を検索。メモ/TODO共通） +sava del : # 特定の日を直接指定して削除 +sava del --deep # 全期間を検索して削除 sava tag ... # 既存のまま（当日分のみ） sava list # 今日の時系列タイムライン sava list / -a / -s / -t # 既存のまま -sava diff ... # 既存のまま +sava diff :...: # 常に : を要求 sava clear ... # 既存のまま # sava carry という独立コマンドは存在しない（自動発火のため） @@ -251,3 +253,50 @@ Added!! 「過去アイテムを直接操作しようとした」エラー案内の重要性が増す。 有効なまま * #4: 影響なし。有効なまま + +## `:` 参照形式の導入 (2026-07-26) + +### 背景 + +Phase A のバグ修正ラウンドで、hash 衝突リトライ（`log.Add` の +`uniqueID`）は同じ日のログ内でしか一意性を保証しないことが分かった +（フォローアップレビュー指摘1）。一方 `index.json` の旧 `Hashes` +フィールド（hash → 日付の逆引き）は全期間で hash が一意という前提で +作られており、`sava diff` がそれを使って hash からアイテムを解決 +していた。日をまたいだ衝突が起きた場合、`diff` が間違った日の +アイテムを黙って指してしまう可能性があった。 + +### 決定事項 + +1. **`sava diff` は常に `:` 形式を要求する**（bare hash は + 受け付けない）。日付が常に明示されるため、hash の一意性を全期間で + 保証する必要が最初からなくなる +2. これに伴い、`index.json` の `Hashes` フィールド・`command.lookup`・ + `Diff` 内の「index が古ければ1回再構築してリトライ」という + self-heal の仕組みを**丸ごと削除**した。`Hashes` の唯一の利用者が + `lookup`（`Diff` からのみ呼ばれる）だったため、`diff` の形式変更で + このサブシステム自体が不要になった +3. **`sava del` はデフォルトで直近 `storagePeriodDays`（30日）分の + ログを検索する**（`clear` の保持期間と同じ窓）。今までの「当日分 + のみ」という制約を撤廃し、直近の消し忘れ・書き間違いに対応しやすく + した + * 検索して一致が0件なら not found、1件ならそこを削除、2件以上 + （複数日にまたがる hash 衝突）なら**エラーを返して何も削除せず + 終了**し、`:` で指定し直すよう案内する + * `:` を直接渡した場合は検索をスキップしてその日に + 直接アクセスする（曖昧性解消の手段にもなる） + * `--deep` フラグで直近30日の窓を外し、全期間を検索できる +4. `sava start` / `sava end` / `sava tag` は今まで通り当日分のみ + （過去ログは不変という原則、およびこれらが TODO のライフサイクル + 管理であり「消し忘れの掃除」とは性質が異なるため、`del` とは + スコープを分けたままにする） + +### 実装メモ + +* `internal/log.HashExists`（旧 `hashExists`）をエクスポートし、 + `command.Del` の複数日検索から再利用した（第三の同種スキャン + ループを増やさないため） +* `command.parseRef`/`resolveRef` を新設し、`Diff`/`Del` 双方が + `:` の解析・解決を共有する +* `list` の各行に `:` をそのまま貼り付けられる形式で + 出す改善は別issue（今回は見送り） From 1c40d69c24aab5e2bca7fd9d73b8b1ae0c13a49f Mon Sep 17 00:00:00 2001 From: rn404 Date: Mon, 27 Jul 2026 09:33:58 +0900 Subject: [PATCH 10/14] feat(command): resolve diff/del via :, drop index.Hashes Diff now takes two : 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 : 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. --- cmd/sava/commands.go | 21 ++--- cmd/sava/root_test.go | 15 +++- internal/command/command.go | 135 ++++++++++++++++++++----------- internal/command/command_test.go | 99 ++++++++++++++++++----- internal/index/index.go | 19 ++--- internal/index/index_test.go | 22 ++--- internal/log/log.go | 8 +- internal/log/log_test.go | 12 +-- 8 files changed, 216 insertions(+), 115 deletions(-) diff --git a/cmd/sava/commands.go b/cmd/sava/commands.go index 8ef806b..64015c3 100644 --- a/cmd/sava/commands.go +++ b/cmd/sava/commands.go @@ -90,33 +90,36 @@ func newTagCommand() *cobra.Command { } func newDelCommand() *cobra.Command { - return &cobra.Command{ - Use: "del ", + var deep bool + cmd := &cobra.Command{ + Use: "del |:", Short: "delete item.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return command.Del(cmd.OutOrStdout(), logfile.Dir(), args[0]) + return command.Del(cmd.OutOrStdout(), logfile.Dir(), args[0], deep) }, } + cmd.Flags().BoolVar(&deep, "deep", false, "search all logs instead of just the last 30 days") + return cmd } func newDiffCommand() *cobra.Command { return &cobra.Command{ - Use: "diff ...", + Use: "diff :...:", Short: "show elapsed time between two items.", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { - hashA, hashB, err := splitDiffArgs(args) + refA, refB, err := splitDiffArgs(args) if err != nil { return err } - return command.Diff(cmd.OutOrStdout(), logfile.Dir(), hashA, hashB) + return command.Diff(cmd.OutOrStdout(), logfile.Dir(), refA, refB) }, } } -// splitDiffArgs accepts either "..." (also "..") as one -// argument or two separate hash arguments. +// splitDiffArgs accepts either "..." (also "..") as one +// argument or two separate ref arguments. func splitDiffArgs(args []string) (string, string, error) { if len(args) == 2 { return args[0], args[1], nil @@ -127,7 +130,7 @@ func splitDiffArgs(args []string) (string, string, error) { return parts[0], parts[1], nil } } - return "", "", fmt.Errorf("expected ... or two hashes") + return "", "", fmt.Errorf("expected ... or two refs") } func newListCommand() *cobra.Command { diff --git a/cmd/sava/root_test.go b/cmd/sava/root_test.go index 6fb55f1..e7bb639 100644 --- a/cmd/sava/root_test.go +++ b/cmd/sava/root_test.go @@ -3,6 +3,8 @@ package main import ( "strings" "testing" + + "github.com/rn404/nippo-cli/internal/model" ) // execute runs the root command with args and returns combined output. @@ -166,22 +168,27 @@ func TestDiffFlow(t *testing.T) { if len(hashes) != 2 { t.Fatalf("hashes = %+v, want 2:\n%s", hashes, list) } + today := model.Today() + refA, refB := today+":"+hashes[0], today+":"+hashes[1] - out := mustExecute(t, "diff", hashes[0]+"..."+hashes[1]) + out := mustExecute(t, "diff", refA+"..."+refB) if !strings.Contains(out, "Diff...") || !strings.Contains(out, "Elapsed: ") { t.Errorf("diff output:\n%s", out) } // Two-argument form works as well. - out = mustExecute(t, "diff", hashes[0], hashes[1]) + out = mustExecute(t, "diff", refA, refB) if !strings.Contains(out, "Elapsed: ") { t.Errorf("two-arg diff output:\n%s", out) } - if _, err := execute(t, "diff", "lonely-hash"); err == nil { + if _, err := execute(t, "diff", "lonely-ref"); err == nil { t.Error("diff without a separator should fail") } - if _, err := execute(t, "diff", hashes[0]+"...no-such-hash"); err == nil { + if _, err := execute(t, "diff", hashes[0], hashes[1]); err == nil { + t.Error("diff with bare hashes (no date) should fail") + } + if _, err := execute(t, "diff", refA+"..."+today+":no-such-hash"); err == nil { t.Error("diff with an unknown hash should fail") } } diff --git a/internal/command/command.go b/internal/command/command.go index 49178db..3b212fd 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -152,38 +152,43 @@ func TagList(w io.Writer, dir string) error { return nil } -// Diff prints the elapsed time between the creation of two items, -// resolving each hash across all daily logs via the index. The index -// is rebuilt once when a hash is not found, so stale cache entries -// heal themselves. -func Diff(w io.Writer, dir, hashA, hashB string) error { - idx, err := index.Load(dir) - if err != nil { - return err +// parseRef splits a ":" reference into its parts. ok is +// false when ref has no colon (a bare hash, not a full reference). +func parseRef(ref string) (date, hash string, ok bool) { + date, hash, found := strings.Cut(ref, ":") + if !found { + return "", ref, false + } + return date, hash, true +} + +// resolveRef finds the item referenced by ":". +func resolveRef(dir, ref string) (model.Item, error) { + date, hash, ok := parseRef(ref) + if !ok { + return model.Item{}, fmt.Errorf("expected :, got %q", ref) } - rebuilt := false - resolve := func(hash string) (model.Item, error) { - for { - if item, ok := lookup(dir, idx, hash); ok { - return item, nil - } - if rebuilt { - return model.Item{}, fmt.Errorf("target item %q is not found", hash) - } - idx, err = index.Rebuild(dir) - if err != nil { - return model.Item{}, err - } - rebuilt = true + file, err := logfile.Stat(dir, date) + if err != nil { + return model.Item{}, err + } + for _, item := range file.Body.Items { + if item.Hash == hash { + return item, nil } } + return model.Item{}, fmt.Errorf("target item %q is not found on %s", hash, date) +} - itemA, err := resolve(hashA) +// Diff prints the elapsed time between the creation of two items, +// each given as ":". +func Diff(w io.Writer, dir, refA, refB string) error { + itemA, err := resolveRef(dir, refA) if err != nil { return err } - itemB, err := resolve(hashB) + itemB, err := resolveRef(dir, refB) if err != nil { return err } @@ -197,26 +202,6 @@ func Diff(w io.Writer, dir, hashA, hashB string) error { return nil } -// lookup finds the item behind hash using the index. A stale entry -// (missing file or hash no longer in it) reports a miss. -func lookup(dir string, idx index.Index, hash string) (model.Item, bool) { - date, ok := idx.Hashes[hash] - if !ok { - return model.Item{}, false - } - - file, err := logfile.Stat(dir, date) - if err != nil { - return model.Item{}, false - } - for _, item := range file.Body.Items { - if item.Hash == hash { - return item, true - } - } - return model.Item{}, false -} - // elapsedBetween returns the absolute distance between the creation // times of two items. func elapsedBetween(a, b model.Item) (time.Duration, error) { @@ -297,9 +282,48 @@ func dedupe(hashes []string) []string { return out } -// Del removes the item matching hash from today's log. -func Del(w io.Writer, dir, hash string) error { - file, err := logfile.Get(dir, "") +// Del removes the item matching ref from a daily log. ref may be a +// bare hash, searched across the last storagePeriodDays days (or +// every day, when deep is true), or a ":" reference that +// is resolved directly, skipping the search. +func Del(w io.Writer, dir, ref string, deep bool) error { + if date, hash, ok := parseRef(ref); ok { + return deleteOn(w, dir, date, hash) + } + hash := ref + + refs, err := logfile.List(dir) + if err != nil { + return err + } + if !deep { + refs = withinStoragePeriod(refs) + } + + var found []string + for _, r := range refs { + file, err := logfile.Stat(dir, r.Name) + if err != nil { + return err + } + if log.HashExists(file.Body.Items, hash) { + found = append(found, r.Name) + } + } + + switch len(found) { + case 0: + return fmt.Errorf("target item %q is not found", hash) + case 1: + return deleteOn(w, dir, found[0], hash) + default: + return fmt.Errorf("hash %q exists on multiple days (%s); specify as :", hash, strings.Join(found, ", ")) + } +} + +// deleteOn removes the item matching hash from the log for date. +func deleteOn(w io.Writer, dir, date, hash string) error { + file, err := logfile.Stat(dir, date) if err != nil { return err } @@ -317,6 +341,23 @@ func Del(w io.Writer, dir, hash string) error { return nil } +// withinStoragePeriod filters refs down to the last storagePeriodDays +// days, mirroring clearOld's retention window. +func withinStoragePeriod(refs []logfile.Ref) []logfile.Ref { + deadline := time.Now().AddDate(0, 0, -storagePeriodDays) + kept := refs[:0] + for _, ref := range refs { + date, err := model.ParseDate(ref.Name) + if err != nil { + continue + } + if !date.Before(deadline) { + kept = append(kept, ref) + } + } + return kept +} + // ListOptions controls the list command behavior. type ListOptions struct { Date string // yyyy-MM-dd; empty means today diff --git a/internal/command/command_test.go b/internal/command/command_test.go index df8f6ef..075bdb7 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/rn404/nippo-cli/internal/index" "github.com/rn404/nippo-cli/internal/logfile" @@ -57,7 +58,7 @@ func TestAddEndDelFlow(t *testing.T) { } out.Reset() - if err := Del(&out, dir, memo.Hash); err != nil { + if err := Del(&out, dir, memo.Hash, false); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Deleted!!") { @@ -67,7 +68,7 @@ func TestAddEndDelFlow(t *testing.T) { t.Errorf("items after Del = %+v, want only the task", items) } - if err := Del(&out, dir, "no-such-hash"); err == nil { + if err := Del(&out, dir, "no-such-hash", false); err == nil { t.Errorf("deleting unknown hash should fail") } } @@ -369,52 +370,112 @@ func TestDiffAcrossDays(t *testing.T) { {Hash: "bbbb2222", Content: "feed the shrimp", CreatedAt: "2026-07-06T12:30:00.000Z", UpdatedAt: "2026-07-06T12:30:00.000Z"}, }) - // No index file exists yet: Diff must rebuild it by itself. var out strings.Builder - if err := Diff(&out, dir, "aaaa1111", "bbbb2222"); err != nil { + if err := Diff(&out, dir, "2026-07-05:aaaa1111", "2026-07-06:bbbb2222"); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Elapsed: 1d 2h 30m") { t.Errorf("Diff output = %q", out.String()) } - if _, err := os.Stat(index.Path(dir)); err != nil { - t.Errorf("Diff should persist the rebuilt index: %v", err) - } // Reversed order measures the same distance. out.Reset() - if err := Diff(&out, dir, "bbbb2222", "aaaa1111"); err != nil { + if err := Diff(&out, dir, "2026-07-06:bbbb2222", "2026-07-05:aaaa1111"); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Elapsed: 1d 2h 30m") { t.Errorf("reversed Diff output = %q", out.String()) } - if err := Diff(&out, dir, "aaaa1111", "no-such-hash"); err == nil { + if err := Diff(&out, dir, "2026-07-05:aaaa1111", "2026-07-05:no-such-hash"); err == nil { t.Errorf("Diff with unknown hash should fail") } + if err := Diff(&out, dir, "aaaa1111", "2026-07-06:bbbb2222"); err == nil { + t.Errorf("Diff with a bare hash (no date) should fail") + } } -func TestDiffHealsStaleIndex(t *testing.T) { +func TestDelSearchesWithinStoragePeriod(t *testing.T) { dir := t.TempDir() - writeDay(t, dir, "2026-07-05", []model.Item{ - {Hash: "aaaa1111", Content: "buy cabbage", CreatedAt: "2026-07-05T10:00:00.000Z", UpdatedAt: "2026-07-05T10:00:00.000Z"}, + recent := time.Now().AddDate(0, 0, -5).Format("2006-01-02") + writeDay(t, dir, recent, []model.Item{ + {Hash: "aaaa1111", Content: "recent memo", CreatedAt: "2026-01-01T00:00:00.000Z", UpdatedAt: "2026-01-01T00:00:00.000Z"}, }) - if _, err := index.Rebuild(dir); err != nil { + + var out strings.Builder + if err := Del(&out, dir, "aaaa1111", false); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), "Deleted!!") { + t.Errorf("Del output = %q", out.String()) + } + file, err := logfile.Stat(dir, recent) + if err != nil { t.Fatal(err) } + if len(file.Body.Items) != 0 { + t.Errorf("item should be removed: %+v", file.Body.Items) + } +} - // The item appears after the index was built: a stale cache miss. - writeDay(t, dir, "2026-07-06", []model.Item{ - {Hash: "bbbb2222", Content: "feed the shrimp", CreatedAt: "2026-07-06T10:00:00.000Z", UpdatedAt: "2026-07-06T10:00:00.000Z"}, +func TestDelIgnoresOldDaysUnlessDeep(t *testing.T) { + dir := t.TempDir() + old := time.Now().AddDate(0, 0, -40).Format("2006-01-02") + writeDay(t, dir, old, []model.Item{ + {Hash: "aaaa1111", Content: "ancient memo", CreatedAt: "2026-01-01T00:00:00.000Z", UpdatedAt: "2026-01-01T00:00:00.000Z"}, }) var out strings.Builder - if err := Diff(&out, dir, "aaaa1111", "bbbb2222"); err != nil { + if err := Del(&out, dir, "aaaa1111", false); err == nil { + t.Error("Del without --deep should not find an item older than the storage period") + } + if err := Del(&out, dir, "aaaa1111", true); err != nil { + t.Fatalf("Del --deep should find the old item: %v", err) + } +} + +func TestDelAmbiguousHashAcrossDays(t *testing.T) { + dir := t.TempDir() + dayA := time.Now().AddDate(0, 0, -3).Format("2006-01-02") + dayB := time.Now().AddDate(0, 0, -5).Format("2006-01-02") + writeDay(t, dir, dayA, []model.Item{ + {Hash: "dup11111", Content: "on day A", CreatedAt: "2026-01-01T00:00:00.000Z", UpdatedAt: "2026-01-01T00:00:00.000Z"}, + }) + writeDay(t, dir, dayB, []model.Item{ + {Hash: "dup11111", Content: "on day B", CreatedAt: "2026-01-01T00:00:00.000Z", UpdatedAt: "2026-01-01T00:00:00.000Z"}, + }) + + var out strings.Builder + if err := Del(&out, dir, "dup11111", false); err == nil { + t.Error("Del with an ambiguous hash should fail without deleting anything") + } + for _, day := range []string{dayA, dayB} { + file, err := logfile.Stat(dir, day) + if err != nil { + t.Fatal(err) + } + if len(file.Body.Items) != 1 { + t.Errorf("neither day should be touched by the ambiguous Del: %s has %+v", day, file.Body.Items) + } + } + + // The : form disambiguates directly. + if err := Del(&out, dir, dayA+":dup11111", false); err != nil { + t.Fatal(err) + } + fileA, err := logfile.Stat(dir, dayA) + if err != nil { + t.Fatal(err) + } + if len(fileA.Body.Items) != 0 { + t.Errorf("day A's item should be gone: %+v", fileA.Body.Items) + } + fileB, err := logfile.Stat(dir, dayB) + if err != nil { t.Fatal(err) } - if !strings.Contains(out.String(), "Elapsed: 1d") { - t.Errorf("Diff should heal the stale index: %q", out.String()) + if len(fileB.Body.Items) != 1 { + t.Errorf("day B's item should be untouched: %+v", fileB.Body.Items) } } diff --git a/internal/index/index.go b/internal/index/index.go index b67cbf7..e43a172 100644 --- a/internal/index/index.go +++ b/internal/index/index.go @@ -1,8 +1,8 @@ // Package index maintains index.json in the log directory: a cache -// mapping tags and item hashes to the daily log files containing them. -// The file is rebuildable from the logs at any time, so it may go -// stale after item deletion; readers should rebuild on a miss instead -// of trusting it blindly. +// mapping tags to the daily log files containing them. The file is +// rebuildable from the logs at any time, so it may go stale after +// item deletion; readers should rebuild on a miss instead of +// trusting it blindly. package index import ( @@ -26,8 +26,6 @@ type Entry struct { type Index struct { // Tags maps a tag to the items carrying it. Tags map[string][]Entry `json:"tags"` - // Hashes maps an item hash to the date (file name) holding it. - Hashes map[string]string `json:"hashes"` } // Path returns the index file location inside the log directory. @@ -38,8 +36,7 @@ func Path(dir string) string { // Build scans every daily log file and returns a fresh index. func Build(dir string) (Index, error) { idx := Index{ - Tags: map[string][]Entry{}, - Hashes: map[string]string{}, + Tags: map[string][]Entry{}, } refs, err := logfile.List(dir) @@ -53,7 +50,6 @@ func Build(dir string) (Index, error) { return Index{}, err } for _, item := range file.Body.Items { - idx.Hashes[item.Hash] = file.Name for _, tag := range item.Tags { idx.Tags[tag] = append(idx.Tags[tag], Entry{Date: file.Name, Hash: item.Hash}) } @@ -67,7 +63,7 @@ func Build(dir string) (Index, error) { // empty index rather than an error: the file is a cache, and callers // are expected to Rebuild on a miss anyway. func Load(dir string) (Index, error) { - empty := Index{Tags: map[string][]Entry{}, Hashes: map[string]string{}} + empty := Index{Tags: map[string][]Entry{}} data, err := os.ReadFile(Path(dir)) if errors.Is(err, os.ErrNotExist) { @@ -84,9 +80,6 @@ func Load(dir string) (Index, error) { if idx.Tags == nil { idx.Tags = map[string][]Entry{} } - if idx.Hashes == nil { - idx.Hashes = map[string]string{} - } return idx, nil } diff --git a/internal/index/index_test.go b/internal/index/index_test.go index 4665dd9..faa44e1 100644 --- a/internal/index/index_test.go +++ b/internal/index/index_test.go @@ -34,8 +34,7 @@ func addTaggedItem(t *testing.T, dir, day, content string, tags []string) string func TestBuildAndRebuild(t *testing.T) { dir := t.TempDir() first := addTaggedItem(t, dir, "2026-07-10", "buy cabbage", []string{"cabbage"}) - second := addTaggedItem(t, dir, "2026-07-11", "feed the shrimp", []string{"shrimp", "pet"}) - plain := addTaggedItem(t, dir, "2026-07-11", "no tags here", nil) + addTaggedItem(t, dir, "2026-07-11", "feed the shrimp", []string{"shrimp", "pet"}) idx, err := Rebuild(dir) if err != nil { @@ -48,11 +47,6 @@ func TestBuildAndRebuild(t *testing.T) { if entries := idx.Tags["cabbage"]; len(entries) != 1 || entries[0].Hash != first || entries[0].Date != "2026-07-10" { t.Errorf("cabbage entries = %+v", entries) } - for hash, date := range map[string]string{first: "2026-07-10", second: "2026-07-11", plain: "2026-07-11"} { - if idx.Hashes[hash] != date { - t.Errorf("Hashes[%s] = %q, want %q", hash, idx.Hashes[hash], date) - } - } // Rebuild persists the index as readable JSON next to the logs. data, err := os.ReadFile(Path(dir)) @@ -63,8 +57,8 @@ func TestBuildAndRebuild(t *testing.T) { if err := json.Unmarshal(data, &reloaded); err != nil { t.Fatalf("index file should be valid JSON: %v", err) } - if len(reloaded.Hashes) != 3 { - t.Errorf("persisted hashes = %+v, want 3", reloaded.Hashes) + if len(reloaded.Tags) != 3 { + t.Errorf("persisted tags = %+v, want 3", reloaded.Tags) } } @@ -73,7 +67,7 @@ func TestBuildEmptyDir(t *testing.T) { if err != nil { t.Fatal(err) } - if len(idx.Tags) != 0 || len(idx.Hashes) != 0 { + if len(idx.Tags) != 0 { t.Errorf("empty dir should yield an empty index: %+v", idx) } } @@ -102,15 +96,15 @@ func TestLoad(t *testing.T) { if err != nil { t.Fatal(err) } - if idx.Tags == nil || idx.Hashes == nil { - t.Errorf("Load should return non-nil maps: %+v", idx) + if idx.Tags == nil { + t.Errorf("Load should return a non-nil map: %+v", idx) } // A broken file is treated as an empty cache, not an error. if err := os.WriteFile(Path(dir), []byte("not json"), 0o600); err != nil { t.Fatal(err) } - if idx, err = Load(dir); err != nil || idx.Tags == nil || idx.Hashes == nil { + if idx, err = Load(dir); err != nil || idx.Tags == nil { t.Errorf("broken index should load as empty: %+v, %v", idx, err) } @@ -123,7 +117,7 @@ func TestLoad(t *testing.T) { if err != nil { t.Fatal(err) } - if idx.Hashes[hash] != "2026-07-11" || len(idx.Tags["go"]) != 1 { + if entries := idx.Tags["go"]; len(entries) != 1 || entries[0].Hash != hash || entries[0].Date != "2026-07-11" { t.Errorf("loaded index mismatch: %+v", idx) } } diff --git a/internal/log/log.go b/internal/log/log.go index 8b2117c..39bf6b9 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -49,14 +49,16 @@ func Add(l *model.Log, content string, isTask bool) (model.Item, error) { // uniqueID generates an ID that none of items already has. func uniqueID(items []model.Item) string { id := generateID() - for hashExists(items, id) { + for HashExists(items, id) { id = generateID() } return id } -// hashExists reports whether any item already has hash. -func hashExists(items []model.Item, hash string) bool { +// HashExists reports whether any item already has hash. Exported so +// callers outside this package (e.g. a multi-day search for a hash) +// can reuse the same check instead of re-scanning by hand. +func HashExists(items []model.Item, hash string) bool { for _, item := range items { if item.Hash == hash { return true diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 96cd421..2011042 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -101,14 +101,14 @@ func TestDeleteRemovesOnlyFirstMatch(t *testing.T) { } func TestHashExists(t *testing.T) { - if !hashExists([]model.Item{{Hash: "x"}}, "x") { - t.Error("hashExists should find a matching hash") + if !HashExists([]model.Item{{Hash: "x"}}, "x") { + t.Error("HashExists should find a matching hash") } - if hashExists([]model.Item{{Hash: "x"}}, "y") { - t.Error("hashExists should not match a different hash") + if HashExists([]model.Item{{Hash: "x"}}, "y") { + t.Error("HashExists should not match a different hash") } - if hashExists(nil, "x") { - t.Error("hashExists on an empty slice should be false") + if HashExists(nil, "x") { + t.Error("HashExists on an empty slice should be false") } } From 6b46a29753a24e6b4f3af38456c3c63b6833b689 Mon Sep 17 00:00:00 2001 From: rn404 Date: Mon, 27 Jul 2026 09:44:34 +0900 Subject: [PATCH 11/14] refactor: fix remaining follow-up review findings (#3-5, #7-8) 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. --- internal/command/command.go | 36 ++++++++++---------- internal/command/command_test.go | 56 ++++++++++++++++++++++++-------- internal/log/log_test.go | 5 +-- internal/view/view.go | 14 +++++--- 4 files changed, 72 insertions(+), 39 deletions(-) diff --git a/internal/command/command.go b/internal/command/command.go index 3b212fd..6c4efec 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -48,18 +48,22 @@ func Add(w io.Writer, dir, content string, opts AddOptions) error { if err != nil { return err } - return persistNewItem(w, dir, file, item) + return persistItem(w, dir, file, item, len(opts.Tags) > 0, view.Added) } -// persistNewItem writes file, confirms item to w, and rebuilds the -// tag index if item carries any tags. Shared by Add and Todo. -func persistNewItem(w io.Writer, dir string, file *logfile.LogFile, item model.Item) error { +// persistItem writes file, reports item to w via confirm, and +// rebuilds the tag index when tagsChanged. tagsChanged must be true +// whenever tags were added OR removed, even if the item ends up with +// zero tags: removing the last tag still leaves a stale index entry +// that needs purging, so "item has tags now" is not a safe substitute +// for "this call touched tags." Shared by Add, Todo, and Tag. +func persistItem(w io.Writer, dir string, file *logfile.LogFile, item model.Item, tagsChanged bool, confirm func(io.Writer, model.Item)) error { if err := logfile.Update(dir, file.Name, file.Body); err != nil { return err } - view.Added(w, item) + confirm(w, item) - if len(item.Tags) > 0 { + if tagsChanged { if _, err := index.Rebuild(dir); err != nil { return err } @@ -95,7 +99,7 @@ func Todo(w io.Writer, dir, content string, opts TodoOptions) error { if err != nil { return err } - return persistNewItem(w, dir, file, item) + return persistItem(w, dir, file, item, len(opts.Tags) > 0, view.Added) } // Tag adds tags to (or removes them from, when remove is true) the @@ -116,15 +120,9 @@ func Tag(w io.Writer, dir, hash string, tags []string, remove bool) error { return err } - if err := logfile.Update(dir, file.Name, file.Body); err != nil { - return err - } - view.TagsUpdated(w, item) - - if _, err := index.Rebuild(dir); err != nil { - return err - } - return nil + // Always true: even removing the last tag needs the index rebuilt + // to purge its now-stale entry. + return persistItem(w, dir, file, item, true, view.TagsUpdated) } // TagList prints every known tag with its item count, refreshing the @@ -242,8 +240,10 @@ func Start(w io.Writer, dir, hash string) error { } // End closes the tasks matching hashes in today's log. Duplicate -// hashes are collapsed to one. All hashes must resolve to open tasks -// or none of them are persisted. +// hashes are collapsed to one. Within this call, all hashes must +// resolve to open tasks or none of them are persisted (this says +// nothing about two concurrent sava processes racing on the same +// file — there is no file locking anywhere in this codebase). func End(w io.Writer, dir string, hashes []string) error { file, err := logfile.Get(dir, "") if err != nil { diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 075bdb7..f53f32f 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -202,14 +202,7 @@ func TestAddOutputsAddedConfirmation(t *testing.T) { // a successful write look like it never happened. func TestAddConfirmsEvenWhenIndexRebuildFails(t *testing.T) { dir := t.TempDir() - if err := os.MkdirAll(dir, 0o700); err != nil { - t.Fatal(err) - } - // A broken sibling log file makes index.Rebuild fail (it scans - // every daily log), independently of today's file. - if err := os.WriteFile(filepath.Join(dir, "2000-01-01.json"), []byte("not json"), 0o600); err != nil { - t.Fatal(err) - } + breakIndexRebuild(t, dir) var out strings.Builder if err := Add(&out, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err == nil { @@ -258,6 +251,33 @@ func TestTagFlow(t *testing.T) { } } +// TestTagRebuildsIndexOnLastTagRemoved guards against a naive shared +// "rebuild if item has tags" helper: removing an item's only tag +// leaves it with zero tags, but the index still must be rebuilt to +// purge that tag's now-stale entry. +func TestTagRebuildsIndexOnLastTagRemoved(t *testing.T) { + dir := t.TempDir() + if err := Add(io.Discard, dir, "buy cabbage", AddOptions{Tags: []string{"onlytag"}}); err != nil { + t.Fatal(err) + } + item := todayItems(t, dir)[0] + + if err := Tag(io.Discard, dir, item.Hash, []string{"onlytag"}, true); err != nil { + t.Fatal(err) + } + if updated := todayItems(t, dir)[0]; len(updated.Tags) != 0 { + t.Fatalf("item should have no tags left: %+v", updated.Tags) + } + + idx, err := index.Load(dir) + if err != nil { + t.Fatal(err) + } + if _, stale := idx.Tags["onlytag"]; stale { + t.Errorf("index should no longer list onlytag: %+v", idx.Tags) + } +} + // TestTagConfirmsEvenWhenIndexRebuildFails mirrors // TestAddConfirmsEvenWhenIndexRebuildFails: Tag must not skip its // confirmation just because the follow-up index.Rebuild fails after @@ -268,12 +288,7 @@ func TestTagConfirmsEvenWhenIndexRebuildFails(t *testing.T) { t.Fatal(err) } item := todayItems(t, dir)[0] - - // A broken sibling log file makes index.Rebuild fail (it scans - // every daily log), independently of today's file. - if err := os.WriteFile(filepath.Join(dir, "2000-01-01.json"), []byte("not json"), 0o600); err != nil { - t.Fatal(err) - } + breakIndexRebuild(t, dir) var out strings.Builder if err := Tag(&out, dir, item.Hash, []string{"food"}, false); err == nil { @@ -347,6 +362,19 @@ func TestListWithTagFilter(t *testing.T) { } } +// breakIndexRebuild writes a corrupt sibling log file so that +// index.Rebuild (which scans every daily log) fails, independently of +// today's file. +func breakIndexRebuild(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "2000-01-01.json"), []byte("not json"), 0o600); err != nil { + t.Fatal(err) + } +} + // writeDay stores items as the log of day, bypassing Add so tests can // control hashes and timestamps. func writeDay(t *testing.T, dir, day string, items []model.Item) { diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 2011042..7caa69e 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -80,8 +80,9 @@ func TestDelete(t *testing.T) { } // TestDeleteRemovesOnlyFirstMatch guards against a hash collision (see -// TestAddAvoidsHashCollision) making Delete remove more than the one -// item it reports. +// TestUniqueIDRetriesOnCollision for the Add-side retry that prevents +// same-day collisions) making Delete remove more than the one item it +// reports. func TestDeleteRemovesOnlyFirstMatch(t *testing.T) { l := model.Log{Items: []model.Item{ {Hash: "dup", Content: "first"}, diff --git a/internal/view/view.go b/internal/view/view.go index 4fed9f9..805f3ac 100644 --- a/internal/view/view.go +++ b/internal/view/view.go @@ -59,19 +59,23 @@ func Added(w io.Writer, item model.Item) { // Deleted prints the deleted item confirmation. func Deleted(w io.Writer, item model.Item) { - fmt.Fprintln(w, "Deleted!!") - fmt.Fprintf(w, "> %s (%s)\n", item.Content, formatTime(item.CreatedAt)) + confirmItem(w, "Deleted!!", item) } // FinishedTask prints the closed task confirmation. func FinishedTask(w io.Writer, item model.Item) { - fmt.Fprintln(w, "Finished!!") - fmt.Fprintf(w, "> %s (%s)\n", item.Content, formatTime(item.CreatedAt)) + confirmItem(w, "Finished!!", item) } // StartedTask prints the started task confirmation. func StartedTask(w io.Writer, item model.Item) { - fmt.Fprintln(w, "Started!!") + confirmItem(w, "Started!!", item) +} + +// confirmItem prints a "