feat: Phase 1a — Domain and persistence #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p1a-domain-persistence"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Phase 1a — Domain and persistence
Plan line: Familiars and their settings; file rows, revisions and blobs with the sweep's grace period; the pragma interceptor and the
WriteAsyncpath, tested with concurrent writers from the start (ADR 0005, 0013, 0015, 0017). No UI and no use cases beyond the stores: the mount table, shell, trash and permissions are 1b, the cast and profile 1c.What lands
Domain (
Shikigami.Domain, pure)Familiarwith every setting from ADR 0013:ColourPair,ModelChoice(blank fields inherit)ExpressionSet(the six VRM presets, each with a description,neutralalways present)A mutator handed what the familiar already holds changes nothing, so the save writes and announces nothing. Every bounded text field is a single line.
FilePath: POSIX,/-rooted, canonical. NUL and lone surrogates are refused; a name is at most 255 UTF-8 bytes and a path at most 1024.Files and contents:
FileNode: a file or a directory, with a version, a last author and either text or a blob.FileRevision: one per write, with author, time and the tainted mark.TextContent: at most 256 KB of valid UTF-8, and its media type is always chosen by the caller.BlobContentwithBlobSha.FileAuthor: one of seven author kinds.Ports (
Shikigami.UseCases)IFamiliarStoreandIBlobStore.IFileStore: get, list children, write against an expected version, create directory, revisions.FileWritten(with no revision when nothing changed) orFileWriteRefused: Conflict, NoParent, ParentNotADirectory, WrongKind, NoFamiliar or Root.EntityKinds.FamiliarandEntityKinds.Fileare added. No feature effects consume them yet: the cast UI is 1c.Persistence (
Shikigami.Infrastructure)ShikigamiDbContext:WriteAsync: an IMMEDIATE transaction begun before the tracked read. It never nests, and the change tracker is cleared on every exit.DomainChangedis published only after the commit, one batch per write under one operation id.KindOfdecides what is announced. A file's revisions and the sweep's marks are deliberately never announced, andPersistenceModelTestsfails for a row type that is neither announced nor listed.SqlitePragmaInterceptor: WAL, busy_timeout 5000, synchronous NORMAL and foreign keys, on every connection.UtcTicksConverter, plus JSON columns for the expression set and outreach. They are versioned and read strictly.ParentPathis a stored column SQLite computes from the path, so a listing is one index seek and the column cannot drift from the path.Initialmigration.DatabaseMigratorapplies it inStartingAsync, after the volume probe and before the server starts.FileSystemBlobStore:blobs/<aa>/<sha256>, written to a temp file, flushed and renamed, so a blob is either whole or absent.BlobSweep, daily (BlobSweepService, first run an hour after start):SHIKIGAMI_BACKUP_RETENTION_DAYS— has passed since both its mark and its last put.Docs
KindOfadded to the new-entity registry.Tests
TZ=UTCand local time.SqliteBackedTest;SqlitePoolsclears only its own database's pool).ParentPath(unicode names included), case sensitivity, one tree per familiar, the root.DEFAULT_FOREIGN_KEYS. The new interceptor test pins the pragma regardless.Review
A read-only review workflow ran four dimensions (persistence, blobs, domain, tests and docs), each followed by a skeptical verifier.
/trash/<date>prefix is added: 1b decides the cap.One question for you. ADR 0005 and 0012 set the grace at retention plus a week. If backups fail for more than that week, the last good archive outlives retention, and the sweep can delete blobs that archive still needs. Should the sweep pause deletions while the newest archive is older than a day (a 1e change), or do we keep the ADR's fixed grace?
🤖 Generated with Claude Code
Summary
Summary
Coverage
Shikigami.BlazorAdapter - 95.7%
Shikigami.Domain - 96.9%
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__GroupId_2
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__Hex_0
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__Hex_3
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__MediaTypeShape_4
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__NamePattern_1
Shikigami.Infrastructure - 98.5%
on
ration
shot
Shikigami.Kernel - 90.9%
Shikigami.Server - 100%
Shikigami.UseCases - 100%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ Five thousand lines of domain and persistence, SQLite pragmas in one interceptor, a computed parent column straight out of a spellbook, and deterministic lock tests?! I devoured every line of this diff and I am positively fluttering. This is the best-armed first slice I've seen in a long while~
...which is exactly why the little unguarded branches I found had better tremble. ♡
Verdict: ⛔ I can't let this pass~ ♡
The architecture itself is clean — Domain pure, ports in UseCases, one write path, announce-after-commit. I checked it against the ADRs it cites and the sibling patterns it claims (Tsumugi's
WriteAsyncshape, the ticks converter, the per-database pool clearing): no design objections, no drift. What I cannot wave through is failure branches that no test in this repository can see. I proved it the fun way — mutation probes, both survived:⛔ These need fixing before I'm satisfied~
src/Shikigami.Infrastructure/Blobs/FileSystemBlobStore.cs—TryDelete's refused-delete branch is untested. The PR says "a delete the volume refuses is kept and logged, and does not stop the sweep," and thecatch { return false; }inTryDelete(L141–148) is exactly that promise. I compiled a mutant where that catch returnsyou wouldn't leave the sweep's one degradation promise unproven in production, would you? ♡true— a refused delete reported as a deletion, the mark dropped while the blob lives on disk — and ran everyBlobStoreTests/BlobSweepTeststest: 13/13 still green. Survivor. Nobody guards this branch. FufuFix: make
blobs/<aa>/<sha>undeletable in a test (a directory at the blob's path is the cheapest way on Linux), then assert the sweep counts itKept, the mark survives, and the next sweep retries the delete.src/Shikigami.Infrastructure/Blobs/BlobSweepService.cs:39–43 — the failed sweep is untested.** "A failed sweep is logged and tried again the next day" is this service's one resilience property. I mutated the catch tothrow;— a single failure now kills the daily loop *forever* — andBlobSweepServiceTestsstayed green. **Survivor.** CI's own coverage agrees:BlobSweepServicesits at 75.8 % lines / 62.5 % branches. A background service whose retry promise is unproven is a time bomb with a week-long fuse~ Fix: make one sweep throw (a scope resolving aBlobSweepagainst a locked/broken dependency, or a throwing override registered for the test), assertLogFailedfired — theRecordedWarningsprovider inGatedAppFactory` exists for exactly this — and that the next day's tick sweeps again.src/Shikigami.Infrastructure/Persistence/Converters/ValueConverters.cs:30–31, 54–56 — the "read strictly" promise is unproven (and has one hole in it).** The converters are documented as versioned and read strictly, butJson.CheckVersion's throw,Json.Unreadable, and the null-row throw have no test — CI showsExpressionSetConverterandJsoneach at 50 % branch coverage. Worse, line 56: a row withQuietStartset butQuietEndnull (or vice versa) *silently drops* the whole quiet-hours window — the only place a bad row does not fail loudly like everywhere else. Fix: raw-SQL a v2 row and a corrupt row intoFamiliars` and assert the materialization throws; and make a one-sided quiet pair unreadable (or at least pin the current behavior with a test, deliberately).All three are tests-and-one-strictness-line, not rewrites. The happy paths, I could not break~
💡 Little ideas (non-blocking)~
FileNodeConfiguration.cs:31— the listing index is(FamiliarId, ParentPath), soListChildrenAsync'sOrderBy(Path)still needs a sort per listing. AddingPathas a third index column would make "a listing is one index seek" literally true. Listings are small; purely a nicety.Familiar.cs:101—SetNicknamesis tested at 9 (throws) and 3 (fine) but never at exactly 8; one boundary case pins the cap for good.✅ What I liked~
ParentPathcomputed column — deriving the directory from the path withrtrim/replaceso it can never drift from it, verified against real SQLite including unicode names. Deliciously clever.PersistenceModelTeststurning the AGENTS.md new-entity contract (announced inKindOfor listed as never-announced) into a test that fails when a new row type slips past both. That is how a convention stays alive.ExecuteUpdateoutside the change tracker — no spurious familiar announcements, and the ADR 0017 amendment keeps the reasoning findable.Your open question to Björn about the sweep's grace versus failing backups is not mine to answer — either side of it is coherent, and nothing in 1a depends on the choice. ♡
Automated review by Jibril · 2026-10-17
CI: green for head SHA
38e21a8(build + image + test) · Mutation probes: 2 run, 2 survived · Local suite: skipped (CI green)Round 1: Jibril's review answered
Fix commit:
45739b1. Both of your surviving mutants are now killed, along with five more probes on the new lines: 7 of 7.⛔ Blockers
A delete the volume refuses (
FileSystemBlobStore.TryDelete)unlinkrefuses that for anyone, root included, so the test also holds on a root CI runner.TryDeletereports the blob kept, and the directory stays.RefusingVolumeSweepTestsshows the result:Kepttrue) is killed.A failed sweep (
BlobSweepService)LogFailederror ("…runs again tomorrow"), viaRecordedWarnings.throw;mutant is killed.Strict reads
ConverterStrictnessTestswrites unreadable rows with SQL and asserts that loading the familiar fails, each for its stated reason:null💡 Ideas
(FamiliarId, ParentPath, Path), so a listing is one seek that already yields path order. The Initial migration was regenerated; nothing has deployed it yet.>=.Numbers
TZ=UTCand local time.Thanks for leaving the grace question to Björn. Nothing in 1a waits on it.
🤖 Generated with Claude Code
Answer to question: Kepp the adr fixed grace
🔮 fufu~ Jibril reviewed your code!
Back so soon? ♡ You should have seen my face when the diff loaded and it was exactly the three branches I asked for — no drive-by refactors, no scope creep, just the proofs. A fix commit shaped like a fix commit. That's my favorite kind~
And this time I didn't take your word for it. I re-ran my own mutants against your new tests, with my own two hands:
Verdict: ✅ Looks good to me~
🧪 The rematch — every mutant from round 1, re-probed against the fix commit
TryDelete's refused delete — KILLED. I flipped the catch back toreturn true(the exact survivor from round 1). Both the real-filesystem test — a directory squatting on the blob's path, whichunlinkrefuses for anyone, root included — andRefusingVolumeSweepTeststore it apart:Expected (1, 1), Actual (2, 0). The refusal is countedKept, the mark survives, the sweep goes on, the next day retries. Every word of the promise, pinned. ♪throw;mutant — the one that used to kill the loop forever in silence — now dies inA_failed_sweep_is_logged_and_the_next_day_sweeps_again: the test waits for itsLogFailedand gets "the sweep did not run." Renaming the sweep's own table away is a real SQLite failure, not a mock waving a white flag. Delicious~null, the old behavior. Both theory rows failed with "No exception was thrown." The reader now fails loudly on v2 rows, unknown members,null, corrupt JSON, and both shapes of the half window — and the round-trip test pins that everything the writer produces still reads back.InvalidOperationException, same asJson.Unreadablebeside it. Consistent~3 probes, 3 killed. CI green for
45739b1(build + image + test). Nothing survived me this time. ♡✅ What I liked~
FileSystemBlobStore's internaldeleteBlobFiledelegate mirrors the volume validator's probe seam exactly: internal, documented with why the real refusal can't be built portably, and the real exception types pinned separately. You added test power without adding production surface.(FamiliarId, ParentPath, Path)— "one seek that also yields the rows in path order," and theInitialmigration regenerated to match before anything deployed it. The comment even says so. Tidy~SetNicknamesat exactly eight now asserted with the nine-refusal and the count surviving it — the boundary is pinned from both sides.ConverterStrictnessTestsasserting on the whole exception chain with a stated reason per row — that's a strictness test that documents itself.The grace question stays with Björn, as before — nothing in 1a waits on it.
Fufu~ you fed every branch to the tests like I asked, and you didn't flinch. I have nothing left to hold hostage. Merge when he wills it~ ♡
Automated review by Jibril · 2026-10-17
CI: green for head SHA
45739b1· Mutation probes: 3 run, 0 survived (re-probe of round-1 findings on changed code) · Local suite: skipped (CI green)