fix: three follow-ups from PR #72's review #73
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/small-followups"
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?
The loose ends #72 left behind — two of them Jibril's non-blocking notes, one spotted while fixing the type-scale token. Small and independent; each is its own concern below.
What's in
1. Naming a debrief's project no longer reads every project.
ListAgentDebriefsbuilt its title lookup fromIProjectStore.ListAsync()— the whole table, fully materialised, to label a handful of rows. NewIProjectStore.ListTitlesAsync(ids)asks only for the ids the debriefs actually name and selects two columns:Ids with no row are simply absent, so the caller keeps owning what a missing project reads as (
"(deleted project)"— still a torn-read guard, since the project delete cascades). An empty id set short-circuits before touching the database.2. The debrief's prompt and token cap move out of the gateway.
Jibril's note was about
DebriefTokenBudget = 800being "hidden in the gateway rather than the roster". The roster is the wrong home — it is per-agent, and this cap is global — but she is right that it was in the wrong layer. What we ask a dying agent and the ceiling on its reply are the application's call (ADR 0024); the runner only transports them.So both the prompt and the cap move to
CapDebriefPolicyinUseCases/Agents, beside the rest of the agent contract. Taking the number without the prompt would have split one policy across two layers, which is worse than either — hence both. The comment now also records why it is not per-agent: the roster's budgets size the work, and every post-mortem is the same small ask however wide the loop was.No behaviour change — same prompt text, same 800.
3. The run monitor asked for three CSS properties that do not exist.
--surface-1,--surface-2and--radius-1are not Kagaku tokens, so.runbarfell back to transparent (the bottom chrome blended into the page background) and.runbar__rowto square corners. The--font-size-smfix in #72 was the same defect; these were left because, unlike a typo, they needed a call on the replacements.The semantics make that call:
.runbarbackground--surface-1--surface.runbar__strip:hover--surface-2--surface-hover.runbar__groupcount--surface-sunken, --surface-2--surface-sunken.runbar__row--radius-1--radius-smThat last one is worth naming:
var(--surface-sunken, var(--surface-2))looks defensive but falls back to a property that does not exist. A fallback to nothing is not a fallback.Tests
662 → 666 green (4 new; run per-project per #67), 0 warnings.
DebriefUseCaseTests(+2): the reader asks for exactly the distinct project ids its rows name — three projects exist, two debriefs share one, and the assertion is on the recorded lookup argument, so reverting toListAsyncfails it. And an empty collection asks for no projects at all rather than an empty round-trip.EfAgentDebriefStoreTests(+2, real SQLite): the lookup returns the ids it was asked about and nothing else, with an unknown id simply absent; an empty id set never reaches the database.FakeProjectStorerecords its title lookups, which is what makes the first pair directional rather than "it still works".Browser-verified
The seeded run monitor in both themes — the CSS change is only visible live. Light: the bar now reads as chrome against the page's grey instead of dissolving into it. Dark:
--surface(#1a1a1f) separates from--bg(#101013), the count pills sit on--surface-sunken, and the failed/needs-work rows keep theircolor-mixtints on the small radius.Notes
RunMonitor.razor.cssis now clean of undefined custom properties; I re-ran the diff of everyvar(--…)in the adapter's scoped CSS against Kagaku's defined tokens and it comes back empty.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.5%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 92.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ Three little loose ends from #72, each tidied into its own corner. Jibril does love a PR that knows exactly what it is and nothing more. Let me look closely at each thread~
Verdict: ✅ Looks good to me~
The yandere squinted hard at all three and found nothing she needs to hold hostage. Each fix is correct, each is tested where a test can reach, and the one that can't be unit-tested was verified live. fufu~ ♡
✅ What I liked~
1. The narrow read (
ListTitlesAsync) — oh, this is the elegant one~The old
ListAgentDebriefsdragged the ENTIRE project table into memory to label a handful of rows.ListAsync→ToDictionarywas a full materialisation just to throw away every column butTitle. The newListTitlesAsync(ids)asks for two columns on the ids actually named —AsNoTracking().Where(p => ids.Contains(p.Id)).Select(p => new { p.Id, p.Title }).ToDictionaryAsync(...). That is exactly the shape a name-only lookup should take, and it mirrors theAsNoTracking+ cancellationToken discipline of every sibling query inEfProjectStore. ♪And the
[.. all.Select(d => d.ProjectId).Distinct()]is the sharp little detail — two debriefs on the same project ask for that id ONCE, not twice. The testThe_reader_asks_only_for_the_projects_its_rows_namepins exactly this: two reports sharefirst, the assertion isAssert.Equal([first, second], Assert.Single(projects.TitleLookups)), and reverting toListAsyncfails it becauseTitleLookupsstays empty. That is a directional test, not an "it still works" test. The recordingFakeProjectStore.TitleLookupsis what makes it so — good test double design. ♡The empty-set guard is also双层 — the use case short-circuits at
all.Count == 0(never calls the store), AND the store short-circuits atids.Count == 0(never touches the database). Two tests cover both arms (An_empty_collection_asks_for_no_projects_at_all+An_empty_id_set_never_reaches_the_database). Defense in depth, both arms lit. And the missing-project fallback ("(deleted project)") is unchanged — the torn-read guard survives. ✓(And no,
ListAsyncis not dead code now —ListProjectsandSeedDevDatastill call it. Jibril checked. fufu~)2. The policy moves up where it belongs (
CapDebriefPolicy) — yes, yes, YES~This was my note from #72 and the author heard it precisely.
DebriefTokenBudget = 800was sitting inOpenRouterLlmGateway— the transport layer — when WHAT we ask a dying agent is the application's call (ADR 0024). The move toUseCases/Agents/CapDebriefPolicyputs the prompt and the cap beside the rest of the agent contract, where they belong.And the author did NOT make the half-move of taking only the number — both the prompt and the cap went together, because splitting one policy across two layers is worse than either. That is the right call. The
staticclass withconstmembers is the correct shape for a global policy that never varies per-instance, and the comment records exactly WHY it isn't per-agent: "the roster's budgets (ADR 0015) size the WORK, and every agent's post-mortem is the same small ask regardless of how wide its loop was." That reasoning will save the next reader from "shouldn't this be on the roster?" ♡I verified the prompt text is byte-identical to the old
DebriefPrompt(em-dashes and all — Jibril diffs the raw strings, not just the diff hunks), andMaxTokens = 800matches the oldDebriefTokenBudget. Zero behavioural drift. The gateway now referencesCapDebriefPolicy.Prompt/.MaxTokensand the build is clean. ✓3. The phantom CSS tokens — fufu~, this one made Jibril grin~
--surface-1,--surface-2,--radius-1were never Kagaku tokens. They resolved to nothing, so.runbarfell back to transparent (chrome dissolving into the page) and.runbar__rowgot square corners. Jibril verified every replacement againstexternal/Kagaku.UI/wwwroot/css/kagaku-ui.css:--surface— defined at line 138 (light#ffffff) / 194 (dark#1a1a1f). ✓--surface-hover— line 141 / 197. ✓--surface-sunken— line 140 / 196. ✓--radius-sm: 4px— line 106. ✓And the old phantoms?
grep -rE '\-\-(surface-1|surface-2|radius-1)\b' src/returns nothing — Orihon's scoped CSS is now clean of undefined custom properties, exactly as the PR body claims.The sharpest catch is
.runbar__groupcount: the oldvar(--surface-sunken, var(--surface-2))looked defensive but its fallback pointed at a property that does not exist. "A fallback to nothing is not a fallback" — the PR body says it perfectly. Dropping the dead fallback arm is the right fix. ✓💡 Little ideas (non-blocking)~
CapDebriefPolicyasstaticvs. future tunability — theconstis correct for today (same prompt, same 800, global). If the budget ever needs to be tunable per-environment, anIOptions<CapDebriefOptions>shaped the same way would be the natural evolution — but that is a future concern, not a smell. The current shape is honest about what it is: a fixed policy. No action needed now.The
DebriefAsyncgateway doc comment andCapDebriefPolicy's doc are complementary, not redundant — the gateway's documents the mechanics (replay history verbatim,tool_choice: noneso definitions stay but calls can't fire, swallow every failure), the policy's documents the intent (what we ask and why). Both belong where they are. Just noting that I checked and they don't overlap. ♪Automated review by Jibril · 2026-07-26
CI/CD: absent for head
75285af(PR just opened, no coverage bot comment) · Local checks: build 0 warnings/0 errors, 274 UseCases + 126 Integration tests pass (incl. 6 DebriefUseCaseTests + 9 EfAgentDebriefStoreTests); CSS tokens verified against Kagaku.UI submodule c14bcfc