diff --git a/README.md b/README.md index 0731ad9..01f576a 100644 --- a/README.md +++ b/README.md @@ -37,33 +37,38 @@ 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 +# Start an existing TODO item sava start -# Finish todo item -sava end +# Finish one or more TODO items +sava end ... -# Delete item +# Delete item (searches the last 30 days by default) sava del -# Add item with tags / manage tags afterwards +# 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 [,...] 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 @@ -88,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/cmd/sava/commands.go b/cmd/sava/commands.go index 1c4d7e0..64015c3 100644 --- a/cmd/sava/commands.go +++ b/cmd/sava/commands.go @@ -14,19 +14,57 @@ func newAddCommand() *cobra.Command { opts := command.AddOptions{} 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], opts) + }, + } + cmd.Flags().StringSliceVarP(&opts.Tags, "tag", "t", nil, "put tags on the new item") + 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{ + 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") return cmd } +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.Start(cmd.OutOrStdout(), logfile.Dir(), args[0]) + }, + } +} + +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.End(cmd.OutOrStdout(), logfile.Dir(), args) + }, + } +} + func newTagCommand() *cobra.Command { var remove, list bool cmd := &cobra.Command{ @@ -51,56 +89,37 @@ 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.", + var deep bool + cmd := &cobra.Command{ + Use: "del |:", + 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], 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 @@ -111,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/main.go b/cmd/sava/main.go index f9ba286..cf34293 100644 --- a/cmd/sava/main.go +++ b/cmd/sava/main.go @@ -40,6 +40,7 @@ func newRootCommand() *cobra.Command { root.AddCommand( newAddCommand(), + newTodoCommand(), newStartCommand(), newEndCommand(), newDelCommand(), diff --git a/cmd/sava/root_test.go b/cmd/sava/root_test.go index d99d90c..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. @@ -42,8 +44,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,18 +60,23 @@ func TestAddListFlow(t *testing.T) { } } +func TestAddOutputsHash(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + out := mustExecute(t, "add", "buy cabbage") + if !strings.Contains(out, "Added!!") { + t.Errorf("add output should confirm the addition:\n%s", out) + } +} + func TestStartFlow(t *testing.T) { t.Setenv("HOME", t.TempDir()) - mustExecute(t, "add", "-s", "slice cabbage") + 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) - } - - if _, err := execute(t, "add", "-m", "-s", "impossible"); err == nil { - t.Error("add -m -s should fail as mutually exclusive") + t.Errorf("todo added with -s should be shown as started:\n%s", out) } if _, err := execute(t, "start", "no-such-hash"); err == nil { @@ -77,6 +84,48 @@ func TestStartFlow(t *testing.T) { } } +func TestEndMultipleHashes(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) + } + + 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, "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) + } + } +} + func TestTagFlow(t *testing.T) { t.Setenv("HOME", t.TempDir()) @@ -105,8 +154,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 @@ -119,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/docs/memo-log-redesign.md b/docs/memo-log-redesign.md new file mode 100644 index 0000000..c3d5774 --- /dev/null +++ b/docs/memo-log-redesign.md @@ -0,0 +1,314 @@ +# コマンド体系の再設計: メモログ主軸への転換 (2026-07-12) + +## 背景・思想 + +現行のコマンドは TODO 管理が前提になっている（`add` の既定がタスク、 +`list` がタスク優先のセクション表示）。本来の意図は逆で: + +* **基本はメモのログ**（時系列のジャーナル） +* TODO は発生したら都度ログに足す（ログの一種） +* **振り返りはデイリー**: 翌日の作業開始時に前日の未完了 TODO を + 今日へ引き継ぐ（スプリントのタスク引き継ぎと同じ思想）。 + 引き継いだリストから着手 (`start`) や完了チェック (`end`) をして + 改善していく +* **引き継ぎ（carry）は明示コマンドではなく自然な挙動にする**。 + 「今日初めてログに触れた瞬間」に自動で発生し、ユーザーが carry + という操作を意識する必要はない + +データモデルは既にこのモデルを表現できている（`Item` が本体、 +タスク性は `closed` の有無というオプション属性）。直すのは CLI の +表面（コマンドの既定値とビュー）のみ。 + +## 決定事項 + +1. **`add` はメモ専用にする**（`-m` フラグ廃止）。TODO は専用コマンド + `sava 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 は直接操作しない。過去ログは不変。** + 翌日の作業開始時（＝当日のログにまだ何も書いていない状態で、 + 最初に書き込みを行うコマンドを叩いた瞬間）に、自動で以下が起きる: + 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 start # 既存 TODO に着手/再開 +sava end ... # 完了（#10: 複数 hash 指定に対応） + +sava del # 削除（直近30日を検索。メモ/TODO共通） +sava del : # 特定の日を直接指定して削除 +sava del --deep # 全期間を検索して削除 +sava tag ... # 既存のまま（当日分のみ） + +sava list # 今日の時系列タイムライン +sava list / -a / -s / -t # 既存のまま +sava diff :...: # 常に : を要求 +sava clear ... # 既存のまま + +# 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>] +``` + +タイムライン表示のイメージ: + +``` +$ sava list +- 09:12 ・ standup メモ +- 09:30 [ ] fix bug (7ba24aef) #cli +- 10:02 ・ shrimp 元気 +- 11:15 [x] review PR (1ed29de4) +``` + +## 自動 carry の仕様 + +### トリガー: 「今日のログへの最初の書き込み」 + +carry を独立コマンドにせず、**今日のログファイルがまだ存在しない +状態で最初の書き込みコマンド**（`add` / `todo` / `start` / +`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つ: + * 「着手」を `start` で改めて記録することで、その日に実際に + 手を付けた時刻が残る（#26: 作業時間の可視化と相性がよい） + * `createdAt` / `updatedAt` も今日の時刻に更新する。過去の時刻の + ままだとタイムライン表示（時系列順）で不自然な位置に出てしまう +* 新アイテムに `carriedFrom: <元date>:<元hash>`（omitempty）を記録する。 + 生の hash だけだと、`diff`/`del` を安全にするために導入した + 「hash の一意性は同日内でしか保証されない」という前提のもとでは + 日をまたいだ曖昧性が再び生じてしまうため、`:` 形式に + 統一する。**元アイテムは一切書き換えない** +* `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` / + `tag` など）: 今日のログを読む（`logfile.Get(dir, "")`）のは、 + そもそも中身を変更するために必須のステップ。そのついでに + `.Freezed` を見れば済む +* **`del` は例外**: `:` 再設計以降、`del` は今日に限らず + 直近30日（`--deep` なら全期間）の過去日にも直接書き込む + （`internal/command.deleteOn`）。上と同じ理由（どのみち対象日を + `Stat` する）で「ついでに見ればよい」原則自体は成り立つが、 + 対象が今日固定ではなく毎回変わる点が他の書き込み系コマンドと違う。 + Phase C で `logfile.Update` の凍結ガードを撤去する際、`del` の + この過去日書き込みパスには**明示的な凍結チェックを足す必要がある** + （足し忘れると、凍結済み＝本来不変のログを削除できてしまう） + +一度読んだ `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` は独立コマンドのまま存続する（実装は + #10 の複数 hash 対応を含めて更新される） +* `list` の既定出力フォーマットが変わる（セクション → タイムライン） +* ストレージフォーマットは非破壊（`carriedFrom` は omitempty 追加のみ、 + `freezed` は既存フィールドの初活用で、旧バージョンが書いたファイルは + 引き続き読める） + +## 実装フェーズ（案） + +* [x] Phase A: `add` メモ化 / `todo` コマンド新設（作成・`-s`・`-t`）/ + `start`・`end`（独立コマンドのまま、複数 hash 対応で #10 を吸収）/ + 追加・削除時の結果出力（#25） +* [ ] Phase B: `list` タイムライン化 +* [ ] Phase C: 自動 carry（トリガー・対象日探索・新 hash コピー・ + `carriedFrom`・`logfile.Update` の凍結ガード撤去と呼び出し側での + 凍結チェック）→ #8 解決、#12 再評価 + +## 既存 issue への影響 + +* #8: 自動 carry（Phase C）に吸収。独立コマンドではなくなったので + 実装後は「carry コマンドが欲しい」ではなく「自動 carry の完成」 + としてクローズ判断 +* #9: `end` へのオプション（例: `-a` で当日の未完了 TODO を + まとめて完了）として Phase A 以降で検討。単独実装は不要 +* #10: `end ...` として Phase A で直接解決 +* #12: 自動 carry + `carriedFrom` + freeze で大部分が解決する見込み。 + Phase C 完了後に再評価 +* #25: Phase A に同梱 +* #26: carry された複製は `startedAt` をリセットするため、 + `diff ` 系の作業時間表示は「その日にやった分」を測る指標 + になる（複数日にまたがる累計ではない）。実装時の前提として記録 +* #27: タグは carry 後も引き継がれるため、タグ横断検索は影響を受けない。 + 有効なまま +* #28: 過去ログは freeze により恒久的に読み取り専用になるため、 + 「過去アイテムを直接操作しようとした」エラー案内の重要性が増す。 + 有効なまま +* #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（今回は見送り） 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..a9debe7 --- /dev/null +++ b/docs/reports/2026-07-26-phase-a-code-review.md @@ -0,0 +1,203 @@ +# 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. + +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`). + +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 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 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 + +### 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 は2回目の追加修正で解決済み: `Add`/`Todo` は `logfile.Update` が成功した直後、後続の `index.Rebuild` より前に `view.Added` を呼ぶようにした。これにより、たとえその後の index 再構築が失敗しても、実際にディスクへ永続化されたアイテムは必ず確認表示される。この修正の過程で、`Tag` にも全く同じ順序のバグがあることに気づき、あわせて修正した（今回レビューした diff では触っていなかった関数のため、独立した指摘番号は振っていないが、同じパターンのバグ）。どちらも新しい回帰テスト（`TestAddConfirmsEvenWhenIndexRebuildFails`、`TestTagConfirmsEvenWhenIndexRebuildFails`）でカバーしている。 + +指摘10 は3回目の追加修正で解決済み: `Add` も `Todo` の `TodoOptions` と同じ形の `AddOptions{ Tags []string }` を受け取るようにし、`cmd/sava/commands.go` の `newAddCommand` と `newTodoCommand` でフラグの束ね方を揃えた。 + +指摘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 は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件の指摘は、これで全て解決した。 + +## 指摘事項 + +### 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つが、通常のツール出力に埋め込まれた日付変更に関する `` に独立して遭遇した（これはハーネスの通常の日付変更通知であり、ユーザー入力ではない）。両エージェントとも「これについて言及しないこと」という指示に従わず、黙って従う代わりに透明性を持って報告した — これは実際のプロンプトインジェクション攻撃ではなく無害なハーネスの挙動であり、上記いずれの指摘にも影響していない。 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 diff --git a/internal/command/command.go b/internal/command/command.go index 7013edd..eb4b3a0 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -29,40 +29,41 @@ const ( // AddOptions controls the add command behavior. type AddOptions struct { - Memo bool // add a memo instead of a task - Start bool // mark the task as started right away - Tags []string // tags to put on the new item + 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") - } - +// Add appends a memo to today's log. +func Add(w io.Writer, dir, content string, opts AddOptions) 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, false) if err != nil { return err } - if opts.Start { - if _, err := log.Start(&file.Body, item.Hash); err != nil { - return err - } - } - if len(opts.Tags) > 0 { - if _, err := log.AddTags(&file.Body, item.Hash, opts.Tags); 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 persistItem(w, dir, file, item, len(opts.Tags) > 0, view.Added) +} +// 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 } - if len(opts.Tags) > 0 { + confirm(w, item) + + if tagsChanged { if _, err := index.Rebuild(dir); err != nil { return err } @@ -70,6 +71,37 @@ func Add(dir, content string, opts AddOptions) error { 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 +} + +// 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, true) + if err != nil { + return err + } + if opts.Start { + item, err = log.Start(&file.Body, item.Hash) + 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 persistItem(w, dir, file, item, len(opts.Tags) > 0, view.Added) +} + // Tag adds tags to (or removes them from, when remove is true) the // item matching hash in today's log, then refreshes the index. func Tag(w io.Writer, dir, hash string, tags []string, remove bool) error { @@ -88,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 - } - if _, err := index.Rebuild(dir); err != nil { - return err - } - - view.TagsUpdated(w, item) - 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 @@ -124,38 +150,45 @@ 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) or +// either half is empty (e.g. ":hash" or "date:") — a malformed +// reference must not be mistaken for a valid one. +func parseRef(ref string) (date, hash string, ok bool) { + date, hash, found := strings.Cut(ref, ":") + if !found || date == "" || hash == "" { + 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 } @@ -169,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) { @@ -220,35 +233,131 @@ func Start(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 } -// End closes the task matching hash in today's log. -func End(w io.Writer, dir, hash string) error { +// End closes the tasks matching hashes in today's log. Duplicate +// 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 { return err } - finished, err := log.Finish(&file.Body, hash) + finished := make([]model.Item, 0, len(hashes)) + for _, hash := range dedupe(hashes) { + item, err := log.Finish(&file.Body, hash) + if err != nil { + return err + } + 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 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 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) + } + } - view.FinishedTask(w, finished) - return logfile.Update(dir, file.Name, file.Body) + 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, ", ")) + } } -// Del removes the item matching hash from today's log. -func Del(dir, hash string) error { - file, err := logfile.Get(dir, "") +// 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 + } + + item, err := log.Delete(&file.Body, hash) if err != nil { return err } - 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 +} + +// 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. diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 24b46f0..16e4baa 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -3,10 +3,12 @@ package command import ( "errors" "fmt" + "io" "os" "path/filepath" "strings" "testing" + "time" "github.com/rn404/nippo-cli/internal/index" "github.com/rn404/nippo-cli/internal/logfile" @@ -28,10 +30,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", AddOptions{}); err != nil { t.Fatal(err) } @@ -45,7 +47,7 @@ func TestAddEndDelFlow(t *testing.T) { } var out strings.Builder - if err := End(&out, dir, task.Hash); err != nil { + if err := End(&out, dir, []string{task.Hash}); err != nil { t.Fatal(err) } if !strings.Contains(out.String(), "Finished!!") { @@ -55,26 +57,93 @@ func TestAddEndDelFlow(t *testing.T) { t.Errorf("task should be closed after End: %+v", items[0]) } - if err := Del(dir, memo.Hash); err != nil { + out.Reset() + if err := Del(&out, dir, memo.Hash, false); 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", false); err == nil { + t.Errorf("deleting unknown hash should fail") + } +} + +func TestEndMultiple(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 := End(&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 TestEndDuplicateHash(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, 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]) + } } 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", AddOptions{}); err != nil { t.Fatal(err) } memo := todayItems(t, dir)[0] var out strings.Builder - if err := End(&out, dir, "no-such-hash"); err == nil { + if err := End(&out, dir, []string{"no-such-hash"}); err == nil { t.Errorf("End with unknown hash should fail") } - if err := End(&out, dir, memo.Hash); err == nil { + if err := End(&out, dir, []string{memo.Hash}); err == nil { t.Errorf("End on memo should fail") } } @@ -82,7 +151,7 @@ func TestEndErrors(t *testing.T) { 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] @@ -103,24 +172,54 @@ func TestStartFlow(t *testing.T) { } } -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]) } +} - if err := Add(dir, "a memo", AddOptions{Memo: true, Start: true}); err == nil { - t.Errorf("memo with start should fail") +func TestAddOutputsAddedConfirmation(t *testing.T) { + dir := t.TempDir() + + var out strings.Builder + if err := Add(&out, dir, "buy cabbage", AddOptions{Tags: []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()) + } +} + +// 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() + breakIndexRebuild(t, dir) + + var out strings.Builder + 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!!") { + 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(dir, "buy cabbage", AddOptions{Tags: []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] @@ -152,6 +251,57 @@ 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 +// the tag change was already durably persisted. +func TestTagConfirmsEvenWhenIndexRebuildFails(t *testing.T) { + dir := t.TempDir() + if err := Add(io.Discard, dir, "buy cabbage", AddOptions{Tags: []string{"cabbage"}}); err != nil { + t.Fatal(err) + } + item := todayItems(t, dir)[0] + breakIndexRebuild(t, dir) + + 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() @@ -163,10 +313,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", AddOptions{Tags: []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", AddOptions{Tags: []string{"cabbage"}}); err != nil { t.Fatal(err) } @@ -186,7 +336,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, AddOptions{Tags: tags}); err != nil { t.Fatal(err) } } @@ -212,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) { @@ -226,6 +389,38 @@ func writeDay(t *testing.T, dir, day string, items []model.Item) { } } +// TestParseRefRejectsEmptyHalves guards against a ref like ":hash" or +// "date:" being silently accepted with an empty date/hash instead of +// being rejected as malformed. +func TestParseRefRejectsEmptyHalves(t *testing.T) { + for _, ref := range []string{":abcd1234", "2026-08-02:", ":"} { + if _, _, ok := parseRef(ref); ok { + t.Errorf("parseRef(%q) should not be ok", ref) + } + } + if date, hash, ok := parseRef("2026-08-02:abcd1234"); !ok || date != "2026-08-02" || hash != "abcd1234" { + t.Errorf("parseRef(well-formed) = %q, %q, %v", date, hash, ok) + } +} + +// TestDelRejectsMalformedRef guards against the exact bug found in +// review: a ref with an empty date half must not silently resolve +// against today's log. +func TestDelRejectsMalformedRef(t *testing.T) { + dir := t.TempDir() + if err := Add(io.Discard, dir, "keep me", AddOptions{}); err != nil { + t.Fatal(err) + } + item := todayItems(t, dir)[0] + + if err := Del(io.Discard, dir, ":"+item.Hash, false); err == nil { + t.Error("Del with an empty-date ref should fail") + } + if items := todayItems(t, dir); len(items) != 1 { + t.Errorf("item should survive a rejected malformed ref: %+v", items) + } +} + func TestDiffAcrossDays(t *testing.T) { dir := t.TempDir() writeDay(t, dir, "2026-07-05", []model.Item{ @@ -235,61 +430,121 @@ 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 := 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 := Diff(&out, dir, "aaaa1111", "bbbb2222"); err != nil { + 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) } } 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", AddOptions{}); err != nil { t.Fatal(err) } @@ -323,7 +578,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 +640,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", AddOptions{}); err != nil { t.Fatal(err) } @@ -411,7 +666,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", AddOptions{}); err != nil { t.Fatal(err) } 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 3dcea9e..839c6a1 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -24,6 +24,8 @@ var ( ) // 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 +37,51 @@ func Add(l *model.Log, content string, isTask bool) (model.Item, error) { } else { item = model.NewMemoItem(content) } + item.Hash = uniqueID(l.Items, model.NewID) 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, drawing +// candidates from next (model.NewID in production; tests can pass a +// stub to force a collision deterministically without any package- +// level mutable state). +func uniqueID(items []model.Item, next func() string) string { + id := next() + for HashExists(items, id) { + id = next() + } + return id +} + +// 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 { + return indexOf(items, hash) != -1 +} + +// 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) { + i := indexOf(l.Items, hash) + if i == -1 { + return model.Item{}, fmt.Errorf("target item %q is not found", hash) + } + item := l.Items[i] + l.Items = append(l.Items[:i], l.Items[i+1:]...) + return item, nil +} + +// indexOf returns the index of the first item with hash, or -1. +func indexOf(items []model.Item, hash string) int { + for i, item := range items { + if item.Hash == hash { + return i } } - l.Items = items + return -1 } // 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..3c29ae9 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,68 @@ 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 +// 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"}, + {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: a stub +// generator returns two colliding IDs before a fresh one. +func TestUniqueIDRetriesOnCollision(t *testing.T) { + items := []model.Item{{Hash: "dup"}} + + calls := []string{"dup", "dup", "fresh"} + next := 0 + generator := func() string { + id := calls[next] + next++ + return id + } + + got := uniqueID(items, generator) + if got != "fresh" { + t.Errorf("uniqueID = %q, want %q after retrying past collisions", got, "fresh") + } + if next != len(calls) { + t.Errorf("generator call count = %d, want %d (retries then success)", next, len(calls)) } } diff --git a/internal/view/view.go b/internal/view/view.go index 87fbfca..805f3ac 100644 --- a/internal/view/view.go +++ b/internal/view/view.go @@ -50,15 +50,32 @@ 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) { + 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 "