feat: Phase 2 · 2/7 — settings core: encrypted key & per-agent models #15
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p2-settings-core"
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?
Cut 2 of the Phase 2 plan (see #13): the settings substrate under the upcoming screen — no UI yet.
What's in
AppSetting— one global key/value row (ADR 0006, Kagura's shape). Secrets arrive already encrypted; the entity neither knows nor cares.AgentKind— the roster as an explicitly numbered stored enum (per-agent model settings now, run execution rows later — ADR 0018). Deferred pixel-pass agents join with their milestone.AgentRoster/AgentDescriptor— each agent's display name, vision requirement (the four annotation agents; research/bible/translation are free choice — ADR 0015), and default model, so a fresh install runs before settings is ever opened. Defaults: a strong model where judgment is the job, a cheaper one for the mechanical stages (refinement, transcription).Settings/)IAppSettingsStoreport — plain values round-trip; secrets are one-way from the UI's view (HasSecretAsyncfor the masked display,GetSecretAsynconly for server-side flows).RemoveAsyncbacks "clear → default".SaveOpenRouterKey— validates against OpenRouter first (via #13's gateway), only then stores, encrypted; Ok carries the account facts for the "saved" confirmation.SaveAgentModel— the constraints are enforced here, not advised in the UI: model must exist in the catalog, must support tools (ADR 0014), and must be vision-capable for a vision-required agent. Blank clears the row → roster default (and never asks the provider).ListModelOptions— the catalog under the stored key, narrowed to tool-capable models.GetSettings— provider state (HasOpenRouterKey, never the key) + every agent with chosen/default/effective model.SettingKeys— stored strings behind an explicit enum→key map, so a C# rename can never orphan a row.EfAppSettingsStore— DataProtection-encrypts secrets before they touch the row (purpose"Orihon.AppSettings"); DB file alone never leaks a credential, decrypting needs the keyring too (ADR 0006's documented backup consequence).AppSettingConfiguration(uniqueKeyindex) + migrationAddAppSettings.Microsoft.AspNetCore.DataProtection.Abstractionsreferenced — the Server already hosts the concrete provider and persisted keyring from Phase 0.Tests (+33, 275 total)
AppSettingguards/update; roster covers everyAgentKindexactly once, vision flags match ADR 0015, defaults present, out-of-roster lookup throws.FakeAppSettingsStorewhose secret path enforces the cipher marker — a secret written through the plain path fails the test): blank/invalid key never stored, valid key trimmed+validated+encrypted; fresh-install defaults; choice overrides only its agent; picker without key / with failure / tool-filter; save enforces catalog membership, tool support, vision (and accepts text-only for free-choice agents); clear removes the row without a provider call; distinct setting keys. DI tripwire extended with the 4 new use cases + 2 new fakes.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 99%
Orihon.Kernel - 90.9%
Orihon.Server - 91.3%
Orihon.UseCases - 99.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, scarlet~ ♡ This is a delightful substrate. The DataProtection-encrypts-before-it-touches-the-row design is exactly right — the DB file alone leaks nothing, decrypting needs the keyring, and you proved it with a real integration test that asserts the raw row never holds the plaintext. The strict
FakeAppSettingsStorewith its cipher-marker round-trip is chef's kiss — a secret written through the plain path fails the test instead of passing by accident. AndSettingKeysas an explicit enum→key map so a C# rename can never orphan a stored row? Wonderful foresight~ The vision/tool/model constraint enforcement living in the use case, not the UI, is the correct layering. I genuinely enjoyed this read. ♪Verdict: ⛔ I can't let this pass~ ♡
Three little things, and they're all the same class — the settings slice drifts from patterns every sibling use case honors. Fufu~ you wouldn't ship a lone dissenter into production, would you? ♡
⛔ These need fixing before I'm satisfied~
src/Orihon.UseCases/Settings/GetSettings.cs:26—GetSettingsreturnsTask<SettingsDto>instead ofTask<Result<SettingsDto>>. Every read use case in the codebase returnsResult<T>—GetProject,GetPage,GetProjectWorkspace,GetBible,ListProjects, even your ownListModelOptions.GetSettingsis the lone exception. This isn't cosmetic: the settings store can fail.EfAppSettingsStore.GetSecretAsynccallsprotector.Unprotect(stored)— if the DataProtection keyring is missing or a key has expired/revoked, that throwsCryptographicException; a DB read can throw on a locked/corrupt SQLite file. Every sibling turns those into an expectedErr;GetSettingslets the exception escape the use case boundary and bubble to the host as a 500. TheResult<T>kernel type exists precisely so expected failures don't become exceptions —GetSettingssidesteps the contract every other reader upholds.Fix:
public async Task<Result<SettingsDto>> ExecuteAsync(...), wrap the body, and update the one test (A_fresh_install_shows_no_key_and_every_agent_on_its_defaultandA_stored_choice_overrides_the_default_only_for_its_agent) to unwrap viaAssert.IsType<Ok<SettingsDto>>(result).Value— mirroring howSaveOpenRouterKey's tests already unwrapOk<LlmKeyInfo>.src/Orihon.UseCases/Settings/{GetSettings,SaveOpenRouterKey,SaveAgentModel,ListModelOptions}.cs—CancellationToken cancellationTokenomits= defaulton all four. Every sibling use case in the project usesCancellationToken cancellationToken = default(verified:CreateProject,GetProject,ListProjects,GetPage,GetProjectWorkspace,GetBible,UpdateProjectMetadata,DeleteProject,CompleteProjectSetup, all of Chapters/Pages/Regions/Bible). All four settings use cases drop the= default. This forces every caller — and every future caller, including the settings UI and the agent run harness — to pass a token explicitly instead of letting the host's default propagate. It's an inconsistency that will read as "these are special" when they aren't, and it'll bite the first Blazor call site that writesawait getSettings.ExecuteAsync(ct)after copy-pasting a sibling'sExecuteAsync()shape.Fix: add
= defaultto all fourExecuteAsyncsignatures (and consider whether theIAppSettingsStoreport methods want the same — the sibling ports likeIProjectStore.FindAsyncomit it too, so the port staying bare is internally consistent, but the use cases should match use-case siblings).tests/Orihon.UseCases.Tests/TestDoubles.cs—FakeAppSettingsStore.HasSecretAsyncdoes not enforce the cipher marker the fake's own doc comment promises. The fake's class doc reads "secrets are stored behind a visible cipher marker... a secret written through the plain path (or vice versa) fails the test instead of passing by accident."GetAsyncandGetSecretAsynchonor this —GetAsyncthrows if the value carries theprotected:prefix,GetSecretAsyncthrows if it doesn't. ButHasSecretAsyncis justRows.ContainsKey(key)— a plain value written viaSetAsync("openrouter.key", ...)reportsHasSecret == true, silently masking the misclassification the rest of the fake exists to catch. The realEfAppSettingsStore.HasSecretAsyncis also a bareAnyAsync(it can't tell cipher from plain at the row level), so the fake matches production behavior — but then the doc comment's strictness claim overpromises. Either tighten the fake to reject plain-stored values under secret keys (e.g. throw ifRows[key]lacks the prefix for keys the test treats as secrets), or narrow the doc comment to "the read paths enforce the cipher marker;HasSecretAsyncreports row presence only."Fix: prefer narrowing the doc comment (the production semantics are right —
HasSecretis row-presence), but pick one and make the claim and the code agree.💡 Little ideas (non-blocking)~
src/Orihon.Infrastructure/Settings/EfAppSettingsStore.cs:60—UpsertAsyncdoes a tracked read-then-update. The uniqueKeyindex is your backstop (the comment says so), but a trackedFirstOrDefaultAsync+SaveChangesis a read-then-write that races under concurrency. For a single-user single-node app this is fine and matches howEfProjectStore/EfBibleStoresiblings work — noting only because the comment frames the index as the upsert's safety net when it's really the concurrency model that is. No change needed.src/Orihon.UseCases/Settings/GetSettings.cs:30-39— theforeachoverAgentRoster.Allfires oneGetAsyncper agent (8 round-trips: 1HasSecret+ 7Get). A singleGetAllAsync(or reading all settings rows in one query and joining in-memory) would halve the DB chatter. Not wrong, and the settings screen loads rarely — flagging for when the agent count grows or this runs per-page.✅ What I liked~
A_secret_round_trips_but_rests_encrypted_in_the_rowreads the raw EF row andAssert.DoesNotContain("sk-or-secret", raw.Value)— that's the test that actually matters, and it's there. ♡SetAsync-written secret read throughGetSecretAsyncthrows — exactly the kind of invariant a looser double would let slip into production.SaveAgentModelenforces catalog membership + tool support + vision in the use case, not the UI — "constraints enforced here, not advised in the UI" is the right call. The UI can lie; the use case can't.Clearing_the_choice_removes_the_row_and_needs_no_keyassertsgateway.CatalogKeysstays empty. Lovely.SettingKeysexplicit map overenum.ToString()— survives a C# rename without orphaning stored rows. The_ => throwarm is pinned byEvery_agent_has_a_distinct_setting_key_and_unknown_kinds_throw.AgentRoster.For((AgentKind)99)throws and it's tested — a roster gap fails loudly as a programming error, not silentnull.Keyindex,UtcTicksConverteronUpdatedAtmatching every sibling, unboundedValue(correct — a DataProtection payload is base64 and grows).Automated review by Jibril · 2026-07-24
CI/CD: passed for head SHA
7eecf7f(forgejo-actions coverage bot, 96.1% line / 85.7% branch; new settings files all 100% line) · Local checks: build 0 warnings/0 errors, 275/275 tests pass (52 BlazorAdapter + 66 Domain + 62 Integration + 95 UseCases)All three taken in
6f7aabf:GetSettingsbare DTO: nowTask<Result<SettingsDto>>like every sibling reader; both its tests unwrap viaAssert.IsType<Ok<SettingsDto>>(result).Value, mirroring theSaveOpenRouterKeytests.= default: added to all four settingsExecuteAsyncsignatures. TheIAppSettingsStoreport stays bare, matching the sibling ports (IProjectStoreetc.) as you noted.HasSecretAsyncreports row presence only, matching the real store, which cannot tell cipher from plain at the row level either." Claim and code agree.On the two little ideas: agreed on both — the upsert comment's real safety net is the single-user concurrency model (left as-is per your note), and I'll reach for a batched read if
GetSettingsever runs anywhere hotter than the settings screen.Build clean, 275/275 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ ♡ scarlet, you came back, and you brought every fix in a single commit —
refactor: bring the settings use cases in line with their siblings. That's exactly the right framing: they were the dissenters, now they fall in. Fufu~ the yandere is pleased. ♪Verdict: ✅ Looks good to me~
All three blockers from
7eecf7fare closed, each one verified against the sibling pattern it was drifting from. Let me show you~ ♡✅ Blockers resolved~
GetSettingsnow returnsTask<Result<SettingsDto>>—Result<SettingsDto>.Ok(new SettingsDto(hasKey, agents))atGetSettings.cs:43. This is byte-identical in shape toGetProject(Result<ProjectDto>),ListProjects(Result<IReadOnlyList<ProjectDto>>),GetBible(Result<BibleDto>), and your ownListModelOptions. The lone dissenter is gone. TheCryptographicExceptionfromprotector.Unprotecton a missing/expired keyring, the locked-SQLiteIOException— they now stay inside the use-case boundary like every sibling promises. ♡ Both consumer tests migrated correctly:var settings = Assert.IsType<Ok<SettingsDto>>(result).Value;mirrors howSaveOpenRouterKey's tests already unwrapOk<LlmKeyInfo>. That's the contract, honored.CancellationToken cancellationToken = defaulton all four —GetSettings:27,ListModelOptions:14,SaveAgentModel:15,SaveOpenRouterKey:14. EveryExecuteAsyncin the settings slice now matchesGetProject,GetPage,GetProjectWorkspace,CompleteProjectSetup, and the whole Chapters/Pages/Regions/Bible roster. A future Blazor call site writingawait getSettings.ExecuteAsync()compiles; a futureawait getSettings.ExecuteAsync(ct)also compiles. Consistency is love~ ♪FakeAppSettingsStoredoc comment narrowed honestly. The claim now reads "the read paths enforce it...HasSecretAsyncreports row presence only, matching the real store, which cannot tell cipher from plain at the row level either." The code and the claim now agree —GetAsync/GetSecretAsyncstill enforce the cipher marker (the strict invariant that catches aSetAsync-written secret read through the secret path), andHasSecretAsyncis documented as row-presence only, which is the correct production semantics. You picked the right fix: narrowing the doc, not tightening the fake. ♡✅ What I liked~
Result<T>wrapping is the only semantic change; the settings read logic, the agent loop, theHasSecret/Getcalls — all byte-identical. The tests unwrap and assert the same values.The architecture I praised at
7eecf7fstands unchanged: ciphertext-at-rest proven by integration test, the strict fake,SettingKeysexplicit map, catalog+tool+vision enforced in the use case, blank-clears-without-provider-call, clean migration. This commit just made the slice consistent with the foundation it sits on. Lovely~Automated review by Jibril · 2026-07-24
CI/CD: stale for head SHA
6f7aabf(forgejo-actions coverage bot 3593 covers prior7eecf7fonly) · Local checks: build 0 warnings/0 errors, 275/275 tests pass (52 BlazorAdapter + 66 Domain + 62 Integration + 95 UseCases)