feat(core+db+git): memory CRUD/read, soft delete, git wrapper, validation - #6
Merged
Conversation
…wrapper, validation Layers landed together because they compose into the v0.1 write path: git wrapper (T8.1) - isomorphic-git@1.38.4 added; gitAdd / gitCommit / gitRemove with vault-relative path normalization and config-driven author fallback (dennoh / dennoh@localhost) when user.name / user.email is unset DB soft delete (extends T4) - migration v2: ALTER TABLE notes ADD COLUMN deleted_at TEXT - getNoteById / getAllNotes now filter `deleted_at IS NULL` - softDeleteNote stamps deleted_at internally (separate from frontmatter updated_at); hard deleteNote retained for tests / true purge Config - Config.maxFileSizeBytes?: number, default 100 MiB (104_857_600) - readConfig validates positive-integer + fills in default Validation - ContentValidationError carries a stable `code` (validate.content.binary / validate.content.too_large) for the future i18n layer; validateContent rejects NULL bytes then size-cap in that order, measuring Buffer.byteLength(content, "utf8") Memory CRUD + read (T5) - saveMemory / updateMemory / deleteMemory run file → DB → git sequentially; v0.1 trades atomicity for simplicity, scanAndSync reconciles file vs DB at next startup - saveMemory uses writeNote, deleteMemory uses gitRemove (gitAdd cannot stage a deletion in isomorphic-git) - updateMemory writes directly to the stored `path` via writeFileAtomic + serializeFrontmatter to avoid buildNotePath's local-TZ round-trip on createdAt - getNote / listRecent are read-side helpers; listRecent defaults to 10 and excludes soft-deleted via the repository filter Drive-by lint/typecheck fixes on already-merged code - sync.ts: moved `errors` declaration above first use (TS2448) - sync.ts / reindex.ts: `let entries` → `let entries: fs.Dirent[]` - reindex.test.ts: replaced `dates[i]!` non-null assertion with a const-tuple `pairs.entries()` loop Public APIs updated: @/db adds softDeleteNote; @/core adds validateContent + ContentValidationError + saveMemory/updateMemory/ deleteMemory/getNote/listRecent. Internal helpers (mapper, getAllNotes, getCurrentVersion, getNoteById) stay reachable via direct module paths for tests. 230 pass, lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesメモCRUD・Git連携・soft delete基盤
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over caller,git: saveMemory
end
participant caller as 呼び出し元
participant memory as memory.ts
participant validate as validateContent
participant fs as ファイルシステム
participant db as SQLite DB
participant git as isomorphic-git
caller->>memory: saveMemory(db, vaultPath, content)
memory->>validate: validateContent(content, maxSizeBytes)
validate-->>memory: OK or ContentValidationError
memory->>fs: writeNote(YYYY/MM/DD/<id>.md)
memory->>db: insertNote(row)
memory->>git: gitAdd(vaultPath, filePath)
memory->>git: gitCommit(vaultPath, "add <id>")
git-->>memory: sha
memory-->>caller: id
rect rgba(180, 70, 70, 0.5)
Note over caller,git: deleteMemory
end
caller->>memory: deleteMemory(db, vaultPath, id)
memory->>db: getNoteById(id)
db-->>memory: row (deleted_at IS NULL)
memory->>fs: rmSync(filePath)
memory->>db: softDeleteNote(db, id)
memory->>git: gitRemove(vaultPath, filePath)
memory->>git: gitCommit(vaultPath, "delete <id>")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/memory.ts`:
- Around line 134-135: The listRecent function accepts a limit parameter without
validation, allowing invalid values like zero, negative numbers, or non-integers
to be passed through to the slice operation, resulting in incorrect behavior.
Add input validation at the beginning of the listRecent function to ensure limit
is a positive integer before using it. Throw an appropriate error if the limit
value is invalid (zero, negative, or not an integer), as this is a public API
boundary that should enforce strict input requirements per the coding guidelines
for input validation.
- Around line 149-150: The fs.rmSync(filePath) call will throw an exception if
the file has already been deleted, preventing the subsequent softDeleteNote(db,
id) from executing. Modify the fs.rmSync call to use the force option set to
true (fs.rmSync(filePath, { force: true })) so that the deletion succeeds
silently even if the file is missing, allowing the soft delete database
operation to continue executing reliably.
In `@src/core/validate.ts`:
- Line 33: The validateContent function lacks input validation for the
maxSizeBytes parameter, which should be a positive integer. Add explicit
validation at the start of the validateContent function to check that
maxSizeBytes is a positive integer, throwing a descriptive error if the value is
zero, negative, or non-integer. This ensures invalid inputs are caught early at
the entry point rather than producing ambiguous errors downstream.
In `@src/git/commit.ts`:
- Around line 33-40: The code currently falls back to DEFAULT_AUTHOR_NAME and
DEFAULT_AUTHOR_EMAIL using the nullish coalescing operator (??), which can lead
to commits with incorrect author information when git configuration is missing.
Instead of using the ?? fallback, check if both configuredName and
configuredEmail are properly set and throw an explicit error if either is
missing or null/undefined. Only return the configured values when both are
successfully retrieved, ensuring the function fails explicitly when git user
configuration is not properly set up.
- Around line 12-14: The toRelative function currently accepts any filePath
without validating whether the result escapes the vault directory. Add explicit
validation to reject paths containing `..` in the result of path.relative().
After computing the relative path, check if it includes `..` (which indicates an
attempt to escape the vault), and throw an error if detected. This provides
defensive validation at the function level rather than relying solely on
upstream checks like assertSafeNoteId, preventing future callers from
accidentally using unsafe paths. Apply this validation logic at the toRelative
function definition (the anchor site) and ensure consistent error handling
across all callers at the sibling sites as well.
In `@tests/core/memory.test.ts`:
- Around line 117-118: The test at tests/core/memory.test.ts lines 117-118 uses
optional chaining and nullish coalescing (afterRow?.path ?? "") to hide
precondition failures instead of explicitly asserting preconditions. Replace
this pattern by first adding an explicit assertion
expect(afterRow).not.toBeNull() to verify the precondition, then use
non-optional access with afterRow!.path to test the actual behavior. Apply the
same fix pattern to the other affected locations in the same file at lines
148-149, 160-161, and 169-170 where similar optional chaining and nullish
coalescing patterns are used with row or other variables.
In `@tests/core/validate.test.ts`:
- Line 65: The test assertion at line 65 uses a nullish coalescing operator to
check either `e.details.maxFileSizeBytes` or `e.details.maxSizeBytes`, which
masks contract mismatches and fails to strictly validate the error structure.
Replace this ambiguous check with a direct assertion on the correct key that
matches the actual public error contract. If the error contract specifies
`maxSizeBytes`, change the expect statement to check only
`e.details.maxSizeBytes` without any fallback operator, ensuring the assertion
validates the exact error shape and behavior rather than allowing multiple
possible keys.
- Around line 29-38: The test contains an `if (e instanceof
ContentValidationError)` conditional branch that violates the guideline
prohibiting branching in tests. Remove this if statement from three locations in
the file (lines 29-38, 57-67, and 88-96). At each location, after the
`expect(e).toBeInstanceOf(ContentValidationError);` line, add a type assertion
to cast `e` as `ContentValidationError` and then directly perform the subsequent
assertions on `e.code` and `e.message` without any conditional wrapping. This
eliminates the branching while preserving type safety through explicit
assertion.
In `@tests/git/commit.test.ts`:
- Around line 76-90: The test is using the direct git.remove method call for the
deletion case instead of using the gitRemove wrapper function, which means it is
not verifying the public API contract that includes logic like toRelative
conversion. Replace the git.remove call with gitRemove to ensure the deletion
case is tested through the same public wrapper as the add and update cases,
maintaining consistent verification of the complete API behavior across all
operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80ccc30f-6c09-4111-900e-3d02e0fea1ba
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (21)
package.jsonsrc/config/index.tssrc/core/index.tssrc/core/memory.tssrc/core/validate.tssrc/db/index.tssrc/db/reindex.tssrc/db/repository.tssrc/db/schema.tssrc/db/sync.tssrc/git/commit.tssrc/git/index.tstests/cli/config.test.tstests/config.test.tstests/core/memory.test.tstests/core/validate.test.tstests/db/connection.test.tstests/db/reindex.test.tstests/db/repository.test.tstests/db/sync.test.tstests/git/commit.test.ts
…rigor)
- memory.ts: fs.rmSync({force:true}) so deleteMemory is idempotent when the
backing file was already removed externally
- git/commit.ts: toRelative rejects paths whose result contains '..' segments
as defense-in-depth against vault escape
- validate.test.ts: assert e.details.maxSizeBytes directly (no fallback key),
replace if-instanceof branches with `as` casts after toBeInstanceOf
- memory.test.ts: replace `?.x ?? ""` patterns with early-throw on null so
precondition failures aren't hidden behind defaults
- git/commit.test.ts: delete case now goes through the gitRemove public
wrapper instead of calling git.remove directly
Skipped findings: listRecent / validateContent input validation (non-boundary
per CLAUDE.md) and strict-fail on missing git config (explicit product
decision in the prior phase spec).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/git/commit.ts (1)
44-53:⚠️ Potential issue | 🟠 MajorGit author の暗黙フォールバックをやめ、未設定時は明示エラーにしてください。
user.name/user.email未設定時に既定値へフォールバックすると、誤った author 情報でコミットされる可能性があります。ガイドラインどおり明示的に失敗させるべきです。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/git/commit.ts` around lines 44 - 53, The resolveAuthor function currently silently falls back to DEFAULT_AUTHOR_NAME and DEFAULT_AUTHOR_EMAIL when git user.name or user.email are not configured. This can lead to commits with incorrect author information. Instead, modify the function to throw an explicit error when either configuredName or configuredEmail are not set, ensuring that commits require proper git author configuration. Remove the nullish coalescing operators that provide the default fallback values.Source: Coding guidelines
src/core/memory.ts (1)
134-135:⚠️ Potential issue | 🟠 Major
listRecentのlimitを明示検証してください。
limitが0以下や非整数でもそのままsliceされるため、呼び出し入力によって不正な結果になります。公開API境界で正の整数のみ受け付けるべきです。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/memory.ts` around lines 134 - 135, The listRecent function accepts a limit parameter but does not validate that it is a positive integer before passing it to slice, which can produce incorrect results. Add validation at the beginning of the listRecent function to ensure that limit is greater than zero and is an integer value. If the limit is invalid, either throw an error with a clear message or handle it gracefully by using the DEFAULT_RECENT_LIMIT as a fallback.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/core/memory.ts`:
- Around line 134-135: The listRecent function accepts a limit parameter but
does not validate that it is a positive integer before passing it to slice,
which can produce incorrect results. Add validation at the beginning of the
listRecent function to ensure that limit is greater than zero and is an integer
value. If the limit is invalid, either throw an error with a clear message or
handle it gracefully by using the DEFAULT_RECENT_LIMIT as a fallback.
In `@src/git/commit.ts`:
- Around line 44-53: The resolveAuthor function currently silently falls back to
DEFAULT_AUTHOR_NAME and DEFAULT_AUTHOR_EMAIL when git user.name or user.email
are not configured. This can lead to commits with incorrect author information.
Instead, modify the function to throw an explicit error when either
configuredName or configuredEmail are not set, ensuring that commits require
proper git author configuration. Remove the nullish coalescing operators that
provide the default fallback values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3defebd9-016a-4a7e-ab72-b8fdbdf4b6f6
📒 Files selected for processing (5)
src/core/memory.tssrc/git/commit.tstests/core/memory.test.tstests/core/validate.test.tstests/git/commit.test.ts
- git/commit.ts: remove DEFAULT_AUTHOR_NAME/EMAIL and the `??` fallback in resolveAuthor; throw an explicit error when user.name or user.email is unset (or empty). This aligns with the project's "fallbacks are absolutely forbidden" rule that supersedes the prior phase spec's default-author wording — silent placeholders for git identity were exactly the kind of "とりあえず動く" footgun the rule targets. - core/memory.ts: listRecent now rejects non-positive-integer limits. Earlier reasoning leaned on "TS guarantees number at runtime" — incorrect since TS is compile-time only; `slice` silently drops trailing rows on negatives and returns empty on NaN, so a bad limit produces a confusing result rather than a clear error. - tests/git/commit.test.ts: split into "configured identity" and "missing identity" describes; replaced the obsolete "falls back to defaults" test with three failure-path tests (missing name / missing email / both). - tests/core/memory.test.ts: added listRecent guard test (0, -1, 1.5, NaN all throw). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/git/commit.ts`:
- Around line 48-57: The validation checks for configuredName and
configuredEmail only verify if length equals zero, which allows whitespace-only
strings (like " ") to pass through as valid values. Update both validation
conditions to trim the string values first, then check if the trimmed result is
empty. This ensures that strings containing only whitespace are properly
rejected as unconfigured, preventing invalid author information from being used
in commits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea5a963a-0d23-41d2-997c-0d47987b7c96
📒 Files selected for processing (4)
src/core/memory.tssrc/git/commit.tstests/core/memory.test.tstests/git/commit.test.ts
`length === 0` only catches explicit empty strings; values like `" "` slipped through and produced commits with uninformative author entries. Switch to `trim().length === 0` so whitespace-only configuration is rejected with the same "not configured" error. The original (untrimmed) value is discarded — the contract is "reject if effectively empty", not "silently normalize". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v0.1 のライト経路を構成する 4 つの層を同時に積みました：
Git wrapper (T8.1)
isomorphic-git@1.38.4を追加gitAdd/gitCommit/gitRemove：vault 相対パス正規化、user.name/user.emailが未設定ならdennoh/dennoh@localhostにフォールバックgitAddは削除済みファイルを stage できないためgitRemoveを別関数として用意DB ソフトデリート (T4 拡張)
ALTER TABLE notes ADD COLUMN deleted_at TEXTgetNoteById/getAllNotesにWHERE deleted_at IS NULLフィルタsoftDeleteNote：deleted_atを関数内で生成（frontmatterupdated_atと分離）。harddeleteNoteはテスト/真の purge 用に保持Config
Config.maxFileSizeBytes?: number、デフォルト 100 MiBreadConfigで正の整数バリデーション + デフォルト埋め込みValidation
ContentValidationError（code: \"validate.content.binary\" | \"validate.content.too_large\"で i18n 連携想定）validateContent：NULL byte → サイズ上限の順で検査、Buffer.byteLength(content, \"utf8\")で UTF-8 バイト長を計測Memory CRUD + read (T5)
saveMemory/updateMemory/deleteMemory：file → DB → git の順次実行。v0.1 は atomicity を犠牲にし、scanAndSyncが起動時に file vs DB を整合updateMemoryは DB に保存された既存pathに直接writeFileAtomic + serializeFrontmatterで書き込み（writeNoteのbuildNotePath経由だとcreatedAtの local-TZ 解釈で path が flip するリスク）deleteMemoryはgitRemove（gitAddでは deletion を stage できない）getNote/listRecent：読み取り系。listRecentはデフォルト 10、deleted_at IS NULLフィルタ済みスコープ外の必須修正（main の lint/typecheck blocker 復旧）
sync.ts：errors宣言を for-await ループより前に移動（TS2448）sync.ts/reindex.ts：let entries→let entries: fs.Dirent[]（biomenoImplicitAnyLet）reindex.test.ts：dates[i]!非 null 断言を const-tuple +pairs.entries()に置換（biomenoNonNullAssertion）CodeRabbit
公開 API 追記
@/db：softDeleteNote追加@/core：validateContent/ContentValidationError/saveMemory/updateMemory/deleteMemory/getNote/listRecent追加mapper,getAllNotes,getCurrentVersion,getNoteById）は直接モジュール参照のみ可設計判断（タスク文言からの意図的な逸脱）
deleteMemoryの git ステップ：仕様はgitAdd + gitCommitだが、isomorphic-git は削除済みファイルに対する add 不可。gitRemove + gitCommitに変更し、src/git/commit.tsにgitRemoveヘルパーを追加。updateMemoryの書き込み：仕様はwriteNote(...)だが、buildNotePathが createdAt を local-TZ Date getter でYYYY/MM/DDに再分解するため、TZ 差異で path が flip するリスクあり。DB の既存pathに直接writeFileAtomic + serializeFrontmatterで書き込む実装に変更。updateMemoryのdeleted_at != NULL例外：getNoteByIdのフィルタで到達不能。タスク文言自身が補足しているとおり省略（CLAUDE.md の「起き得ないシナリオの validation を書かない」とも整合）。maxFileSizeBytes === undefinedガード：readConfigが DEFAULT_CONFIG から埋め込むため実際には起き得ないが、型ナローイング目的で残置（実害なし、内部エラーで失敗）。Test plan
bun test（230 pass / 17 files / 461 expects）bun run lint（Biome clean）bun run typecheck（tsc --noEmit clean）🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート