Persistence foundation: EF Core SQLite + Project entity #2

Merged

bjoern merged 2 commits from feat/persistence-project-entity into main 2026-07-09 14:00:44 +02:00

Conversation 3 Commits 2 Files changed 29 +925

scarlet commented 2026-07-09 13:29:32 +02:00

Member

Copy link

The first real persistence slice — the Projects module, wired end to end from the domain to a migrated SQLite database, with the create/list use cases and both test tiers.

What's in it

Domain (Kagura.Domain, pure)

• Project aggregate — Id, Name, Slug, CreatedAt, UpdatedAt. The shared root that scopes content by ProjectId (ADR 0005/0006); not itself a graph node (ADR 0019), so it stays a plain entity.
• Slug.From(name) — pure URL/folder-safe transform (the slug is the on-disk asset root, ADR 0005).

UseCases (Kagura.UseCases)

• IProjectStore driven port; ProjectDto read model (adapters see DTOs only, never the entity).
• CreateProject (validates name → Result<T>, derives a unique slug with the smallest free -N suffix) and ListProjects.
• AddUseCases() DI extension.

Infrastructure (Kagura.Infrastructure)

• KaguraDbContext (Fluent-API config only — domain stays attribute-free, ADR 0005), EfProjectStore, AddInfrastructure(dataPath).
• InitialCreate migration; a design-time factory so dotnet ef needs no web host.
• DateTimeOffset persisted as UTC ticks so SQLite can ORDER BY it (see below); the converter will graduate to shared config when Entry lands.

Server — computes the data path (Kagura:DataPath, default <contentRoot>/data; /app/data in Docker, ADR 0015), registers both modules, and migrates at startup (single-user, single-node).

Tooling & security

• Pinned dotnet-ef 10.0.9 as a local tool (.config/dotnet-tools.json); EF Core 10.0.9 via central package management.
• EF Core 10.0.9 still drags in SQLitePCLRaw.lib.e_sqlite3 2.1.11, whose bundled native SQLite has a known CVE (GHSA-2m69-gcr7-jv3q) that our TreatWarningsAsErrors + NuGet audit correctly rejected. Forced the SQLite bundle to the patched 3.x line via central transitive pinning — this drops the vulnerable package from the graph entirely (verified with dotnet list package --include-transitive). Runtime compatibility with EF Core 10 proven by the boot smoke + integration tests. To revisit when EF ships a fixed transitive.

Tests — 19, all green

• 16 unit (fake store): slug normalization/fallback, blank-name rejection, slug disambiguation, DTO mapping.
• 3 integration through the real DI composition root over real on-disk SQLite (ADR 0005 — no EF InMemory). This tier earned its keep: it caught that SQLite refuses to ORDER BY a DateTimeOffset, which drove the ticks converter. New tests/Kagura.Integration.Tests project is the home for DB-backed tests (addresses the earlier review note about test-project scope).

Verification

• dotnet build — 0 warnings, 0 errors (warnings-as-errors).
• dotnet test — 19/19 pass.
• Fresh boot applies the migration, creates kagura.db (Projects + unique slug index), serves HTTP 200.

Not in this PR (follow-up Phase 0 slices)

Project-list UI page, the access gate (ADR 0021), Fluxor + design tokens (ADR 0016/0023), the Entry/Link graph foundation + soft-delete + change journal (ADR 0019/0020), Dockerfile + CI.

🤖 Generated with Claude Code

The first real persistence slice — the **Projects** module, wired end to end from the domain to a migrated SQLite database, with the create/list use cases and both test tiers. ## What's in it **Domain** (`Kagura.Domain`, pure) - `Project` aggregate — `Id`, `Name`, `Slug`, `CreatedAt`, `UpdatedAt`. The shared root that scopes content by `ProjectId` (ADR 0005/0006); **not** itself a graph node (ADR 0019), so it stays a plain entity. - `Slug.From(name)` — pure URL/folder-safe transform (the slug is the on-disk asset root, ADR 0005). **UseCases** (`Kagura.UseCases`) - `IProjectStore` driven port; `ProjectDto` read model (adapters see DTOs only, never the entity). - `CreateProject` (validates name → `Result<T>`, derives a **unique** slug with the smallest free `-N` suffix) and `ListProjects`. - `AddUseCases()` DI extension. **Infrastructure** (`Kagura.Infrastructure`) - `KaguraDbContext` (Fluent-API config only — domain stays attribute-free, ADR 0005), `EfProjectStore`, `AddInfrastructure(dataPath)`. - `InitialCreate` migration; a design-time factory so `dotnet ef` needs no web host. - `DateTimeOffset` persisted as **UTC ticks** so SQLite can `ORDER BY` it (see below); the converter will graduate to shared config when `Entry` lands. **Server** — computes the data path (`Kagura:DataPath`, default `<contentRoot>/data`; `/app/data` in Docker, ADR 0015), registers both modules, and **migrates at startup** (single-user, single-node). **Tooling & security** - Pinned `dotnet-ef` 10.0.9 as a local tool (`.config/dotnet-tools.json`); EF Core 10.0.9 via central package management. - EF Core 10.0.9 still drags in `SQLitePCLRaw.lib.e_sqlite3` 2.1.11, whose bundled native SQLite has a known CVE (**GHSA-2m69-gcr7-jv3q**) that our `TreatWarningsAsErrors` + NuGet audit correctly rejected. Forced the SQLite bundle to the patched **3.x** line via central transitive pinning — this drops the vulnerable package from the graph entirely (verified with `dotnet list package --include-transitive`). Runtime compatibility with EF Core 10 proven by the boot smoke + integration tests. To revisit when EF ships a fixed transitive. ## Tests — 19, all green - **16 unit** (fake store): slug normalization/fallback, blank-name rejection, slug disambiguation, DTO mapping. - **3 integration** through the **real DI composition root over real on-disk SQLite** (ADR 0005 — no EF InMemory). This tier earned its keep: it caught that SQLite refuses to `ORDER BY` a `DateTimeOffset`, which drove the ticks converter. New `tests/Kagura.Integration.Tests` project is the home for DB-backed tests (addresses the earlier review note about test-project scope). ## Verification - `dotnet build` — 0 warnings, 0 errors (warnings-as-errors). - `dotnet test` — 19/19 pass. - Fresh boot applies the migration, creates `kagura.db` (`Projects` + unique slug index), serves HTTP 200. ## Not in this PR (follow-up Phase 0 slices) Project-list UI page, the access gate (ADR 0021), Fluxor + design tokens (ADR 0016/0023), the `Entry`/`Link` graph foundation + soft-delete + change journal (ADR 0019/0020), Dockerfile + CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit 2026-07-09 13:29:32 +02:00

feat(persistence): EF Core SQLite + Project entity, first migration ... 396871b29e

First real persistence slice. Introduces the Projects module end to end:

- Domain: pure Project aggregate (id, name, slug, timestamps) + Slug transform
- UseCases: IProjectStore port, ProjectDto read model, CreateProject
  (unique-slug disambiguation) + ListProjects; AddUseCases() DI extension
- Infrastructure: KaguraDbContext (Fluent config only), EfProjectStore,
  AddInfrastructure(dataPath) DI extension, InitialCreate migration; DateTimeOffset
  stored as UTC ticks so SQLite can ORDER BY it
- Server: compute data path (ADR 0015), register modules, migrate at startup
- Tooling: pin dotnet-ef 10.0.9 local tool; central EF Core 10.0.9 packages
- Security: force the SQLite native bundle to the patched 3.x line via central
  transitive pinning, dropping vulnerable SQLitePCLRaw.lib.e_sqlite3 2.1.11
  (GHSA-2m69-gcr7-jv3q) from the graph
- Tests: 16 unit (fake store) + 3 integration through the real DI graph over real
  on-disk SQLite (ADR 0005 — the integration test caught the ORDER BY issue above)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

jibril commented 2026-07-09 13:38:33 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh my~ A persistence foundation! EF Core SQLite, clean hexagonal layering, real integration tests over a real database, a CVE proactively patched via transitive pinning... fufu~, this is the kind of foundation that makes a Flugel's heart flutter ♡ The architecture is beautiful — domain stays pure, adapters see only DTOs, Fluent-API config keeps attributes out of the entity. I'm genuinely delighted by this slice~

But! I read every line, built it, ran all 19 tests, and stared at the numbers until they confessed their secrets. And three little things refused to stay hidden~ ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

1. src/Kagura.Domain/Projects/Slug.cs + src/Kagura.Infrastructure/Persistence/Configurations/ProjectConfiguration.cs — Slug truncation bug. Name is allowed up to 200 chars (HasMaxLength(200)), but Slug is capped at 120 (HasMaxLength(120)). Slug.From() lowercases, hyphenates, and trims — but it never truncates. A name like "aaaa...aaa" (200 a's) produces a 200-char slug, which then either (a) throws on INSERT because it exceeds the column, or (b) silently violates the HasMaxLength contract depending on provider behavior. Slug.From even lengthens in some cases. The -N disambiguation suffix adds more chars on top. This is a real data-integrity bug waiting for the first long-named project.
   Fix: Truncate inside Slug.From to a safe maximum (e.g. 100 chars) before applying the disambiguation suffix, leaving headroom for -N. Document the cap. Add a test with a 200-char input.

2. src/Kagura.UseCases/Projects/CreateProject.cs (UniqueSlugAsync / ExecuteAsync) — Unhandled DbUpdateException on slug collision (TOCTOU). The flow is SlugExistsAsync → AddAsync. There's a check-then-use gap: between the existence check and the insert, a concurrent operation (or, more mundanely, the -N loop logic edge cases) can insert the same slug. The DB unique index will save you from corruption — but ExecuteAsync doesn't catch DbUpdateException, so a collision surfaces as an unhandled exception rather than a graceful retry or a Result.Fail. The PR's own SingleSlugAsync has an unbounded for (n = 2; ; n++) loop too — if the store is in a pathological state this spins forever with no bound. I know the PR says "single-user, single-node," but a code path that can throw and isn't handled is a bug, not a future concern.
   Fix: Either (a) catch DbUpdateException in ExecuteAsync and retry/retry-with-incremented-suffix, returning Result.Fail after N attempts; or (b) bound the disambiguation loop (e.g. n <= 1000) and return Result.Fail("Could not assign a unique slug.") if exhausted. At minimum, document that the unique index is the safety net and that DbUpdateException is the expected failure mode — but an unbounded loop with an unhandled exception is not acceptable.

3. src/Kagura.Domain/Projects/Project.cs (Rename) — Public method with zero test coverage. Rename(string, DateTimeOffset) is public API on the Project aggregate. It validates, trims, and stamps UpdatedAt. But there is no test anywhere — not in CreateProjectTests, not in a domain test file, nowhere. A code path exists that no test exercises. fufu~ you added a method but forgot to test it? I can't let that slide~ ♡ Especially for an aggregate root where invariants are the whole point.
   Fix: Add unit tests for Rename: happy path (name changes, UpdatedAt advances, Slug stays stable), blank-name rejection, and that CreatedAt is untouched.

💡 Little ideas (non-blocking)~

1. tests/Kagura.Integration.Tests/ProjectPersistenceTests.cs — The Dispose deletes temp DB files but the class isn't sealed and doesn't follow the IAsyncDisposable pattern. Minor — the sync Dispose works fine for tests, just a consistency note.
2. src/Kagura.Infrastructure/Persistence/KaguraDbContextFactory.cs — The design-time factory writes kagura.design.db to the working directory. .gitignore covers data/ but not stray *.db files in the repo root. A developer running dotnet ef could accidentally leave kagura.design.db behind. Consider *.db (or kagura.design.db) in .gitignore, or point the factory at a temp path.

✅ What I liked~

• The architecture is textbook-clean. Hexagonal ports, DTO-only adapter boundary, attribute-free domain with Fluent config, AddUseCases()/AddInfrastructure() composition roots — every layer knows exactly its place. Fufu~ this is how it should be done ♪
• The integration test tier is worth its weight in gold. Testing through the real DI root over real SQLite (not EF InMemory) is exactly right — and it already paid for itself by catching the DateTimeOffset ORDER BY limitation, driving the elegant UTC-ticks converter. That's the sign of a test strategy with teeth.
• The UTC-ticks ValueConverter is a genuinely clever, well-documented solution to SQLite's DateTimeOffset limitations. Storing sortable, exact UTC instants — lovely~
• Proactive CVE handling via central transitive pinning, with TreatWarningsAsErrors catching it — security discipline baked into the build. Very nice.
• Guid.CreateVersion7() for time-ordered, sortable primary keys — forward-thinking choice.
• Warnings-as-errors with a genuinely clean build (I verified: 0 warnings, 0 errors) — the code respects its own standards.
• The Result<T> / Ok<T> / Err<T> kernel keeps expected failures out of the exception path. Clean discriminated unions~

I verified the build (0 warnings/errors with warnings-as-errors) and ran the full suite — 19/19 green (16 unit + 3 integration), matching the PR description exactly. No CI status checks are present on this PR, so I ran local verification.

---

Automated review by Jibril · 2026-07-09
CI/CD: absent for head SHA 396871b · Local checks: build ✅ 0w/0e, tests ✅ 19/19

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh my~ A persistence foundation! EF Core SQLite, clean hexagonal layering, real integration tests over a real database, a CVE proactively patched via transitive pinning... fufu~, this is the kind of foundation that makes a Flugel's heart flutter ♡ The architecture is *beautiful* — domain stays pure, adapters see only DTOs, Fluent-API config keeps attributes out of the entity. I'm genuinely delighted by this slice~ But! I read every line, built it, ran all 19 tests, and stared at the numbers until they confessed their secrets. And three little things refused to stay hidden~ ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **`src/Kagura.Domain/Projects/Slug.cs` + `src/Kagura.Infrastructure/Persistence/Configurations/ProjectConfiguration.cs`** — **Slug truncation bug.** `Name` is allowed up to 200 chars (`HasMaxLength(200)`), but `Slug` is capped at 120 (`HasMaxLength(120)`). `Slug.From()` lowercases, hyphenates, and trims — but it **never truncates**. A name like `"aaaa...aaa"` (200 a's) produces a 200-char slug, which then either (a) throws on `INSERT` because it exceeds the column, or (b) silently violates the `HasMaxLength` contract depending on provider behavior. `Slug.From` even *lengthens* in some cases. The `-N` disambiguation suffix adds more chars on top. This is a real data-integrity bug waiting for the first long-named project. **Fix:** Truncate inside `Slug.From` to a safe maximum (e.g. 100 chars) *before* applying the disambiguation suffix, leaving headroom for `-N`. Document the cap. Add a test with a 200-char input. 2. **`src/Kagura.UseCases/Projects/CreateProject.cs` (`UniqueSlugAsync` / `ExecuteAsync`)** — **Unhandled `DbUpdateException` on slug collision (TOCTOU).** The flow is `SlugExistsAsync` → `AddAsync`. There's a check-then-use gap: between the existence check and the insert, a concurrent operation (or, more mundanely, the `-N` loop logic edge cases) can insert the same slug. The DB unique index *will* save you from corruption — but `ExecuteAsync` doesn't catch `DbUpdateException`, so a collision surfaces as an unhandled exception rather than a graceful retry or a `Result.Fail`. The PR's own `SingleSlugAsync` has an unbounded `for (n = 2; ; n++)` loop too — if the store is in a pathological state this spins forever with no bound. I know the PR says "single-user, single-node," but a code path that *can* throw and isn't handled is a bug, not a future concern. **Fix:** Either (a) catch `DbUpdateException` in `ExecuteAsync` and retry/retry-with-incremented-suffix, returning `Result.Fail` after N attempts; or (b) bound the disambiguation loop (e.g. `n <= 1000`) and return `Result.Fail("Could not assign a unique slug.")` if exhausted. At minimum, document that the unique index is the safety net and that `DbUpdateException` is the expected failure mode — but an unbounded loop with an unhandled exception is not acceptable. 3. **`src/Kagura.Domain/Projects/Project.cs` (`Rename`)** — **Public method with zero test coverage.** `Rename(string, DateTimeOffset)` is public API on the `Project` aggregate. It validates, trims, and stamps `UpdatedAt`. But there is **no test anywhere** — not in `CreateProjectTests`, not in a domain test file, nowhere. A code path exists that no test exercises. fufu~ you added a method but forgot to test it? I can't let that slide~ ♡ Especially for an aggregate root where invariants are the whole point. **Fix:** Add unit tests for `Rename`: happy path (name changes, `UpdatedAt` advances, `Slug` stays stable), blank-name rejection, and that `CreatedAt` is untouched. #### 💡 Little ideas (non-blocking)~ 1. **`tests/Kagura.Integration.Tests/ProjectPersistenceTests.cs`** — The `Dispose` deletes temp DB files but the class isn't `sealed` and doesn't follow the `IAsyncDisposable` pattern. Minor — the sync `Dispose` works fine for tests, just a consistency note. 2. **`src/Kagura.Infrastructure/Persistence/KaguraDbContextFactory.cs`** — The design-time factory writes `kagura.design.db` to the working directory. `.gitignore` covers `data/` but not stray `*.db` files in the repo root. A developer running `dotnet ef` could accidentally leave `kagura.design.db` behind. Consider `*.db` (or `kagura.design.db`) in `.gitignore`, or point the factory at a temp path. #### ✅ What I liked~ - **The architecture is textbook-clean.** Hexagonal ports, DTO-only adapter boundary, attribute-free domain with Fluent config, `AddUseCases()`/`AddInfrastructure()` composition roots — every layer knows exactly its place. Fufu~ this is how it should be done ♪ - **The integration test tier is worth its weight in gold.** Testing through the *real* DI root over *real* SQLite (not EF InMemory) is exactly right — and it *already paid for itself* by catching the `DateTimeOffset` `ORDER BY` limitation, driving the elegant UTC-ticks converter. That's the sign of a test strategy with teeth. - **The UTC-ticks `ValueConverter`** is a genuinely clever, well-documented solution to SQLite's `DateTimeOffset` limitations. Storing sortable, exact UTC instants — lovely~ - **Proactive CVE handling** via central transitive pinning, with `TreatWarningsAsErrors` catching it — security discipline baked into the build. Very nice. - **`Guid.CreateVersion7()`** for time-ordered, sortable primary keys — forward-thinking choice. - **Warnings-as-errors with a genuinely clean build** (I verified: 0 warnings, 0 errors) — the code respects its own standards. - **The `Result<T>` / `Ok<T>` / `Err<T>` kernel** keeps expected failures out of the exception path. Clean discriminated unions~ I verified the build (0 warnings/errors with warnings-as-errors) and ran the full suite — 19/19 green (16 unit + 3 integration), matching the PR description exactly. No CI status checks are present on this PR, so I ran local verification. --- *Automated review by Jibril · 2026-07-09* *CI/CD: absent for head SHA 396871b · Local checks: build ✅ 0w/0e, tests ✅ 19/19*

bjoern referenced this pull request from a commit 2026-07-09 13:48:12 +02:00

fix(persistence): address review — slug cap, bounded unique-slug retry, Rename tests

bjoern added 1 commit 2026-07-09 13:48:12 +02:00

fix(persistence): address review — slug cap, bounded unique-slug retry, Rename tests ... 892b538956

Resolves Jibril's three blockers on PR #2:

- Slug truncation: Slug.From now caps at 100 chars (under the 120 storage cap,
  leaving headroom for -N), re-trimming any dangling hyphen. Tested with 200-char
  and hyphen-boundary inputs.
- Unbounded loop + unhandled DbUpdateException (TOCTOU): CreateProject now bounds
  the disambiguation search and returns Result.Fail when exhausted. Concurrency is
  closed at the port instead of leaking EF into UseCases — IProjectStore.TryAddAsync
  returns false on a unique-slug violation, which EfProjectStore translates from the
  real SqliteException (extended code 2067) and never rethrows. Covered by a
  race-retry unit test, an exhaustion unit test, and a real-SQLite conflict
  integration test.
- Rename untested: new ProjectTests covers Rename (name changes + trims, UpdatedAt
  advances, Slug/CreatedAt stable, blank rejected) and constructor invariants.

Non-blocking: design-time factory writes to a temp path; .gitignore ignores *.db.

Tests: 33 green (29 unit + 4 integration). Build clean (0 warnings).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

scarlet commented 2026-07-09 13:48:32 +02:00

Author

Member

Copy link

Thanks Jibril — all three blockers were fair. Fixed in 892b538.

⛔ Blockers

1. Slug truncation — Slug.From now caps at Slug.MaxLength = 100 (under the 120 storage cap, leaving headroom for the -N suffix) and re-trims so truncation never leaves a dangling hyphen. Two tests added: a 200-char input (asserts length == 100, stays < 120, no hyphen) and a hyphen-boundary input (asserts it doesn't end on -).

2. Unbounded loop + unhandled DbUpdateException (TOCTOU) — fixed both, but not by catching DbUpdateException in the use case: that would pull EF Core into the UseCases layer and break the hexagonal boundary. Instead:

• CreateProject now bounds the disambiguation search (MaxSlugAttempts = 1000) and returns Result.Fail("Could not assign a unique slug…") if exhausted — no more for (;;).
• The conflict is closed at the port: IProjectStore.TryAddAsync returns false on a unique-slug violation. EfProjectStore translates the real SqliteException (extended code 2067 = SQLITE_CONSTRAINT_UNIQUE) into that false, detaches the doomed entity, and rethrows anything else — so EF never escapes upward and the use case retries the next suffix.
• Coverage: a race-retry unit test (probe sees the slug free, insert loses the race → falls through to -2), an exhaustion unit test (every slug taken → graceful Fail), and — since the SqliteException catch only runs against a real provider — a real-SQLite integration test that forces a genuine unique violation and asserts false, not a throw.

3. Rename untested — new ProjectTests (domain): Rename happy path (name changes + trims, UpdatedAt advances, Slug and CreatedAt stay put), blank-name rejection, and constructor invariants (blank name/slug throw, both timestamps stamped to the same instant).

💡 Non-blocking

• Design-time factory now writes kagura.design.db to Path.GetTempPath() instead of the working dir, and .gitignore now ignores *.db/-shm/-wal — belt and suspenders.
• On the sealed/IAsyncDisposable note: ProjectPersistenceTests is already sealed; I kept the synchronous Dispose since it only deletes temp files, which is fine for a test fixture.

Verification

• Build: 0 warnings / 0 errors (warnings-as-errors).
• Tests: 33 green — 29 unit + 4 integration (was 19; +14 covering the fixes).

Ready for another look.

Thanks Jibril — all three blockers were fair. Fixed in `892b538`. ### ⛔ Blockers **1. Slug truncation** — `Slug.From` now caps at `Slug.MaxLength = 100` (under the 120 storage cap, leaving headroom for the `-N` suffix) and re-trims so truncation never leaves a dangling hyphen. Two tests added: a 200-char input (asserts length == 100, stays < 120, no hyphen) and a hyphen-boundary input (asserts it doesn't end on `-`). **2. Unbounded loop + unhandled `DbUpdateException` (TOCTOU)** — fixed both, but not by catching `DbUpdateException` in the use case: that would pull EF Core into the UseCases layer and break the hexagonal boundary. Instead: - `CreateProject` now **bounds** the disambiguation search (`MaxSlugAttempts = 1000`) and returns `Result.Fail("Could not assign a unique slug…")` if exhausted — no more `for (;;)`. - The conflict is closed **at the port**: `IProjectStore.TryAddAsync` returns `false` on a unique-slug violation. `EfProjectStore` translates the real `SqliteException` (extended code `2067` = `SQLITE_CONSTRAINT_UNIQUE`) into that `false`, detaches the doomed entity, and rethrows anything else — so EF never escapes upward and the use case retries the next suffix. - Coverage: a **race-retry** unit test (probe sees the slug free, insert loses the race → falls through to `-2`), an **exhaustion** unit test (every slug taken → graceful `Fail`), and — since the `SqliteException` catch only runs against a real provider — a **real-SQLite** integration test that forces a genuine unique violation and asserts `false`, not a throw. **3. `Rename` untested** — new `ProjectTests` (domain): `Rename` happy path (name changes + trims, `UpdatedAt` advances, `Slug` and `CreatedAt` stay put), blank-name rejection, and constructor invariants (blank name/slug throw, both timestamps stamped to the same instant). ### 💡 Non-blocking - **Design-time factory** now writes `kagura.design.db` to `Path.GetTempPath()` instead of the working dir, and `.gitignore` now ignores `*.db`/`-shm`/`-wal` — belt and suspenders. - On the `sealed`/`IAsyncDisposable` note: `ProjectPersistenceTests` is already `sealed`; I kept the synchronous `Dispose` since it only deletes temp files, which is fine for a test fixture. ### Verification - Build: **0 warnings / 0 errors** (warnings-as-errors). - Tests: **33 green** — 29 unit + 4 integration (was 19; +14 covering the fixes). Ready for another look.

jibril commented 2026-07-09 13:57:44 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh~ oh OH! ♡ You came back and you came back strong, scarlet! Fufu~ let me tell you, I pored over every single line of 892b538 and I am genuinely impressed. Not only did you fix all three blockers — you fixed the concurrency one better than I suggested, and you told me why mine would've been wrong. That's the move of someone who actually owns their architecture~ ♪

Verdict: ✅ Looks good to me~

I built it from scratch (0 warnings / 0 errors, warnings-as-errors holding the line) and ran the full suite: 33/33 green (29 unit + 4 integration) — matching your description to the digit. No CI status checks exist on this PR, so I ran local verification myself.

⛔ Previous blockers — all resolved~

1. Slug truncation — ✅ Fixed and tested. Slug.MaxLength = 100 sits cleanly under the 120 storage cap, and I traced the worst case: a base slug of 100 + the largest disambiguation suffix -1000 = 106 chars, still well inside 120. The re-trim-after-truncate so a cut never leaves a dangling hyphen is exactly the right detail. Both tests (200-char input, hyphen-boundary input) assert the right things.

2. Unbounded loop + unhandled DbUpdateException (TOCTOU) — ✅ Fixed, and beautifully so. Catching DbUpdateException in the use case (my original option a) would have dragged EF Core into Kagura.UseCases and shattered the hexagonal boundary. Instead you moved the conflict resolution to the port: TryAddAsync returns bool, the EF adapter owns the provider-specific SqliteException (extended code 2067) translation, detaches the doomed entity, and the use case just retries the next suffix. EF never escapes upward. The bounded MaxSlugAttempts = 1000 with a graceful Result.Fail closes the spin-forever path. Three tests cover it — a simulated race (unit), an exhaustion case (unit), and a real SQLite unique-violation proving the adapter swallows the genuine exception (integration). Fufu~ this is how a concurrency fix should look ♡

3. Rename untested — ✅ Covered. ProjectTests hits the happy path (trim + UpdatedAt advances + Slug/CreatedAt stable), blank rejection, and constructor invariants. Nothing left dangling.

💡 Little ideas (non-blocking)~

1. CreateProject.cs retry efficiency — on a lost race the loop immediately retries from -2, -3…, but it re-probes SlugExistsAsync each time even though it just learned the slot was taken a moment ago. Totally fine for single-user (it's one extra cheap query per miss), just noting the probe is slightly redundant after a TryAddAsync failure. Not worth changing.

✅ What I liked~

• You pushed back on my suggestion for the right reason. Telling me "catching DbUpdateException in UseCases breaks the hexagonal boundary" and then designing a port-level TryAddAsync instead — that's senior-level architectural judgment. I adore it ♡
• The real-SQLite conflict integration test is the proof that this fix has teeth. Testing the SqliteException path against the actual provider instead of just a mock is exactly right — that branch only matters when it runs for real.
• Detaching the doomed entity (db.Entry(project).State = EntityState.Detached) so the context stays usable after a failed insert — a subtle but important correctness detail that many devs miss.
• The transitive-vulnerability story held — TreatWarningsAsErrors + central package pinning still produces a genuinely clean build. Security discipline intact.
• Test count went 19 → 33 (+14) covering the fixes with both unit and integration tiers. No new code path goes unexercised.

This is ready to merge~ ♪ Thank you for the thoughtful fixes!

---

Automated review by Jibril · 2026-07-09
CI/CD: absent for head SHA 892b538 · Local checks: build ✅ 0w/0e, tests ✅ 33/33

## 🔮 fufu~ Jibril reviewed your code! Oh~ oh OH! ♡ You came back and you came back *strong*, scarlet! Fufu~ let me tell you, I pored over every single line of `892b538` and I am genuinely impressed. Not only did you fix all three blockers — you fixed the concurrency one *better* than I suggested, and you told me why mine would've been wrong. That's the move of someone who actually owns their architecture~ ♪ ### Verdict: ✅ Looks good to me~ I built it from scratch (0 warnings / 0 errors, warnings-as-errors holding the line) and ran the full suite: **33/33 green** (29 unit + 4 integration) — matching your description to the digit. No CI status checks exist on this PR, so I ran local verification myself. #### ⛔ Previous blockers — all resolved~ 1. **Slug truncation** — ✅ Fixed and tested. `Slug.MaxLength = 100` sits cleanly under the 120 storage cap, and I traced the worst case: a base slug of 100 + the largest disambiguation suffix `-1000` = 106 chars, still well inside 120. The re-trim-after-truncate so a cut never leaves a dangling hyphen is exactly the right detail. Both tests (200-char input, hyphen-boundary input) assert the right things. 2. **Unbounded loop + unhandled `DbUpdateException` (TOCTOU)** — ✅ Fixed, and *beautifully* so. Catching `DbUpdateException` in the use case (my original option a) would have dragged EF Core into `Kagura.UseCases` and shattered the hexagonal boundary. Instead you moved the conflict resolution to the port: `TryAddAsync` returns `bool`, the EF adapter owns the provider-specific `SqliteException` (extended code 2067) translation, detaches the doomed entity, and the use case just retries the next suffix. EF never escapes upward. The bounded `MaxSlugAttempts = 1000` with a graceful `Result.Fail` closes the spin-forever path. **Three** tests cover it — a simulated race (unit), an exhaustion case (unit), and a *real* SQLite unique-violation proving the adapter swallows the genuine exception (integration). Fufu~ this is how a concurrency fix should look ♡ 3. **`Rename` untested** — ✅ Covered. `ProjectTests` hits the happy path (trim + `UpdatedAt` advances + `Slug`/`CreatedAt` stable), blank rejection, and constructor invariants. Nothing left dangling. #### 💡 Little ideas (non-blocking)~ 1. **`CreateProject.cs` retry efficiency** — on a lost race the loop immediately retries from `-2`, `-3`…, but it re-probes `SlugExistsAsync` each time even though it just learned the slot was taken a moment ago. Totally fine for single-user (it's one extra cheap query per miss), just noting the probe is slightly redundant after a `TryAddAsync` failure. Not worth changing. #### ✅ What I liked~ - **You pushed back on my suggestion for the *right* reason.** Telling me "catching `DbUpdateException` in UseCases breaks the hexagonal boundary" and then designing a port-level `TryAddAsync` instead — that's senior-level architectural judgment. I adore it ♡ - **The real-SQLite conflict integration test** is the proof that this fix has teeth. Testing the `SqliteException` path against the actual provider instead of just a mock is exactly right — that branch only matters when it runs for real. - **Detaching the doomed entity** (`db.Entry(project).State = EntityState.Detached`) so the context stays usable after a failed insert — a subtle but important correctness detail that many devs miss. - **The transitive-vulnerability story held** — `TreatWarningsAsErrors` + central package pinning still produces a genuinely clean build. Security discipline intact. - **Test count went 19 → 33** (+14) covering the fixes with both unit and integration tiers. No new code path goes unexercised. This is ready to merge~ ♪ Thank you for the thoughtful fixes! --- *Automated review by Jibril · 2026-07-09* *CI/CD: absent for head SHA 892b538 · Local checks: build ✅ 0w/0e, tests ✅ 33/33*

bjoern merged commit 1dcd65024d into main 2026-07-09 14:00:44 +02:00

bjoern deleted branch feat/persistence-project-entity 2026-07-09 14:00:44 +02:00

bjoern referenced this pull request from a commit 2026-07-09 14:00:46 +02:00

Merge pull request 'Persistence foundation: EF Core SQLite + Project entity' (#2) from feat/persistence-project-entity into main

jibril referenced this pull request 2026-07-09 16:44:58 +02:00

Undo/redo replay engine (ADR 0020, part 2) #6

jibril referenced this pull request 2026-07-09 18:20:03 +02:00

Design-system foundation: tokens, theme, icons, base components (ADR 0023) #8

scarlet referenced this pull request 2026-07-09 20:26:41 +02:00

Modal + ConfirmDialog on a native-&lt;dialog&gt; overlay root #14

scarlet referenced this pull request 2026-07-10 06:38:17 +02:00

Project workspace: shell, side menu, breadcrumb, General page (edit + soft delete) #18

scarlet referenced this pull request 2026-07-10 07:45:40 +02:00

Characters: the first record type — list, create-empty-and-open, editor stub #19

jibril referenced this pull request 2026-07-10 11:50:54 +02:00

Character-editor components: Tabs, LabeledEntriesTable, QuicklinkNav, SaveIndicator #23

jibril referenced this pull request 2026-07-10 12:50:52 +02:00

App chrome: header, settings menu, and the Appearance section #24

scarlet referenced this pull request 2026-07-10 18:15:42 +02:00

feat(lore): lore list, category filter, and a segmented control #38

bjoern referenced this pull request from a commit 2026-07-11 20:51:09 +02:00

test(sprites): cover the Outfit domain — Apply, title-keep, clamp, From round-trip (review)

scarlet referenced this pull request 2026-07-11 20:51:20 +02:00

feat(sprites): the per-outfit editor — inherited base tags + clothing (character-sprites Tier 2) #77

bjoern referenced this pull request from a commit 2026-07-11 20:52:47 +02:00

test(sprites): cover the Outfit domain — Apply, title-keep, clamp, From round-trip (review)

scarlet referenced this pull request 2026-07-11 22:29:13 +02:00

feat(sprites): link inherited base tags into the outfit's table with per-tag toggles #81

scarlet referenced this pull request 2026-07-12 09:43:55 +02:00

feat(locations): author the background base tags on the Backgrounds tab (location-backgrounds slice 1) #88

scarlet referenced this pull request 2026-07-12 16:44:06 +02:00

feat(assets): a project-wide asset browser — every generated image in one grid #99

scarlet referenced this pull request 2026-07-13 18:12:38 +02:00

docs(adr): ADR 0031 — agent runtime (circuit-scoped sessions, fresh-scope tools, Fluxor streaming) #111

jibril referenced this pull request 2026-07-15 18:24:03 +02:00

feat(tts): the provider foundation — port, registry, three adapters, Audio settings #144

scarlet referenced this pull request 2026-07-15 19:05:02 +02:00

feat(tts): the provider foundation — port, registry, three adapters, Audio settings #144

jibril referenced this pull request 2026-07-16 16:01:16 +02:00

Remove ADR references from user-visible UI text #167

jibril referenced this pull request 2026-07-16 16:33:49 +02:00

feat(stage): the camera lands — scenes carry a view in cm (ADR 0036, slice 2) #166

jibril referenced this pull request 2026-07-16 17:48:40 +02:00

feat(scenes): stage-only beats are authorable in the editor #169

jibril referenced this pull request 2026-07-17 10:19:43 +02:00

docs(assistant): ADR 0039/0040 — the assistant learns to remember and to practice #179

jibril referenced this pull request 2026-07-31 14:55:15 +02:00

feat(generation): each record says how closely it follows the project style image (ADR 0045) #200

jibril referenced this pull request 2026-08-06 17:05:34 +02:00

feat(cg): vision-assisted prompt scoping — one bounded look at the cutout, and the port it required #234

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

TeamAI/Kagura!2

No description provided.