feat(observability): failures that only the model or the queue page saw now reach the log #172
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/observability-start"
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?
A start on app-wide logging, aimed at the two surfaces where failures currently vanish. Motivated by a live incident:
get_characterrefused every id for a whole conversation, the panel showed only bare ⚠ icons, and the refusal text existed solely inside the model's tool messages — diagnosing it took code archaeology instead of onedocker logs.What now reaches the log
Assistant (
ChatSession)Failedevent).write_scene_linescan carry whole scripts.Generation queue (
ActivityQueue)Tests
ActivityQueuegains anILoggerctor param; the queue's own test suite runs againstNullLoggerunchanged (13 green).🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 89%
Kagura.Domain - 95.8%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 82.8%
Kagura.UI - 95.1%
Kagura.UseCases - 96.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A logging PR that knows exactly where the bodies are buried~ ♡ The incident motivation —
get_characterrefusing every id for a whole conversation, the refusal text living only in the model's tool messages — is precisely the kind of failure that turns a one-minute fix into an afternoon of code archaeology. Making the invisible visible? That's not a feature, that's a kindness to your future self. I read every line of this and verified every claim against the source, including the OpenRouter.Net submodule types. Fufu~ let me show you what I found~Verdict: ✅ Looks good to me~
This is a clean, well-scoped observability slice. Every new log line lands where it should, the new code paths are tested, and the one constructor-signature change (the only realistic footgun) is fully covered. No submodule regression, no broken call sites, no secrets, no threading hazards. I built it and ran the relevant suites.
What I verified line by line~
Type correctness — all the new log arguments check out against the real types. I opened the OpenRouter.Net submodule (at
3369bc05, unchanged from main — no downgrade) and confirmed every property:ChatSession.cs:159—Truncate(completed.Result)—ToolCallCompletedEvent.Resultisstring(non-null),Truncatetakesstring?. ✓ChatSession.cs:158—completed.ToolNameisstring,completed.IsSuccessisbool. ✓ChatSession.cs:183-184—result.ApiError?.Error—AgentResult.ApiErrorisResult<ChatCompletionResponse>.Failure?, andFailureisrecord Failure(string Error, ...). So.Erroris astring(flat human-readable message), not a type name dumped viaToString(). The log will be useful during an incident. ✓ChatSession.cs:196—result.Iterationsisint(the{Iterations}placeholder). ✓turnSw.ElapsedisStopwatch—{Elapsed:c}formats it cleanly. ✓Thread safety of the tool-failure log. The
DelegateProgress<AgentEvent>runs on a background thread (the file documents this at line ~126), so I checked the shared-state access:argsis captured per-call fromargsByCallunder a lock (lines 146-149),Truncateis a pure static, andMicrosoft.Extensions.Logging.ILoggeris thread-safe by contract. No race, no mutation. ✓The constructor-signature change is fully covered.
ActivityQueuegainedILogger<ActivityQueue>. I grepped everynew ActivityQueue(and every DI registration:DependencyInjection.cs:127is a concrete-type singleton (services.AddSingleton<Generation.ActivityQueue>()), so the container auto-resolves the newILogger<>param from the logging pipeline — no registration change needed, and I confirmed the build resolves it. ✓ActivityQueueTests.cs:16, and it's updated to passNullLogger<ActivityQueue>.Instance. ✓IActivityQueueuse a separateInlineActivityQueuefake and are untouched. ✓Queue log (
ActivityQueue.cs~line 248). Runs in the worker's sequentialRunmethod. The log fires afterentry.FinishedAt/entry.Statusare set under the lock, and reads only those values plus the immutableoutcome. No TOCTOU, no reordering. The "queue page only keeps failures until dismissed — the log keeps them" rationale is a genuinely sharp observation about where durability matters. ✓Tests pin the new behavior.
A_failed_tool_call_lands_in_the_server_log_with_its_reasonis exactly the right shape — it drives a real tool round-trip (search → refused with "No project is open…"), thenAssert.Singleon the warning level and checks the message contains the tool name (search), the reason (No project), and the args (Aria). I traced the refusal text back toAssistantTool.cs:42(InProjectScopeAsync→ToolResult.Failure("No project is open…")) — the assertions match the actual runtime text, not wishful thinking. The PR description says this test was "proven red with the log call removed" — that's the TDD discipline I love to see. ♡ListLoggeris not a DRY violation. I grepped all test projects — it's defined exactly once, inChatSessionTests, and no capturing-logger helper exists elsewhere to consolidate it with. (If a second one ever appears, extract it then.)💡 Little ideas (non-blocking)~
ChatSession.cs— theApiErrorwarning duplicates text that already goes to the panel. Not wrong (the log is the persistent copy, which is the whole point of the PR), just noting thatresult.ApiError?.Error ?? "The model call failed."appears in both theLogWarningand theFailedevent. Totally fine for observability — only worth a thought if you later want a singleemitOutcomehelper. No action needed.Truncateat 500 chars is a reasonable default, and the…ellipsis is a nice touch. If you ever log binary-ish or JSON-laden tool results, consider also capping args separately when they're a giantwrite_scene_linespayload — right now both share the same 500 cap, which is fine but means a huge args blob can dominate the line. Truly minor.Automated review by Jibril · 2026-07-17
CI/CD: absent for head
e7c1365(0 comments, no status checks) — ran local checks insteadLocal checks: Build 0 warnings / 0 errors. Relevant suites green —
ChatSessionTests12/12 (incl. newA_failed_tool_call_lands_in_the_server_log_with_its_reason) +ActivityQueueTests13/13 (unchanged vs newNullLoggerctor). Full Integration.Tests run shows 207 failures, but these are purely environmental:SqliteBackedTestwrites per-test DBs to a 512MB/tmptmpfs that fills under the full 530-test load (SQLITE_FULL/ "database or disk is full"). Proven non-code: I re-ran a previously-failingCharactersTeststest in isolation after clearing/tmp→ it passed. Submodule pointers identical to main (no regression):NovelAI.ImageGend7ffd6b,OpenRouter.Net3369bc05,booru-tag-dba0d5b0db.